Introduce path and deprecate hub_con and config_tasks as args to submission_tmpl #196

annakrystalli · 2025-01-29T15:27:12Z

This PR resolves #165 and in the process also resolves #137

The PR introduces hub_path as main argument to submission_tmpl() and deprecates arguments hub_con and config_tasks (#165 & #137). This way, all that is required by the user to create a submission template is the path to a hub directory.

The change is currently back-compatible as the old arguments still work while issuing a deprecation warning. The arguments will be fully removed in a later version (see #197).

Tests were also updated to work with hub_paths while some tests to ensure current back-compatibility were retained. Note that this change requires a fully configured hub to work, but tests that are based on standalone config files have been accomodated by using local mocking of the read_config() function now used internally to read config files.

…f hub_path. Resolves #165 and #137

github-actions · 2025-01-29T15:30:22Z

🚀 Deployed on https://67aa0e193c97d34cdab6f55b--hubvalidations-pr-previews.netlify.app

zkamvar

Thank you! I think the ergonomics of this function are much better with a single argument. It's a much simpler solution than my suggestion from #137. Your method of using a function to switch out the deprecated arguments like that is a nice touch!

I do have a request for an improvement: allow the first argument to take either a hub or a config file and change the argument name to be path.

My rationale is that it improves testing and modeler ergonomics:

tests that are based on standalone config files have been accommodated by using local mocking of the read_config() function now used internally to read config files.

If we allow for a config file as the first argument as well, then these local mocks would not be necessary.

I've described the modeler ergonomics in my comment below.

R/submission_tmpl.R

…rovided

…ing task message.

annakrystalli · 2025-01-31T10:16:24Z

Good idea! Not being able to create a template from a config file was bothering me too. I was just trying to keep the interface as simple as possible. This solution is a good way round it.

I've implemented with minor adaptations. Specifically I ensure we check that path exists first before trying to determine whether it's a directory or file. I also use an fs function (which is already an import) as it's name is more self descriptive.

I've also updated the test to remove local bindings, done a bit of reorganising for clarity and added a few more specific tests that were being tested in a more high-level fashion but not clearly indicated as such.

Re:

I often hear from modelers that they do not often work directly with the hub on their machine when creating their model outputs.

This is interesting and makes me wonder, how to they ensure they have access to an up to date version of the config locally?

zkamvar

Thank you for the updates! I have two final suggestions:

update the NEWS item
add a catch so that non-json files throw an informative error

I also added some comments when I was tracing the changes in the snapshot outputs.

To answer the question you asked about modelers:

I often hear from modelers that they do not often work directly with the hub on their machine when creating their model outputs.

This is interesting and makes me wonder, how to they ensure they have access to an up to date version of the config locally?

That's a good question, if they have an internet connection, they could do something like this and get the file since jsonlite::fromJSON() understands URLs

cfg <- "https://raw.githubusercontent.com/reichlab/variant-nowcast-hub/refs/heads/main/hub-config/tasks.json"
submission_tmpl(path = cfg, ...)

zkamvar · 2025-02-03T16:07:56Z

NEWS.md

@@ -1,5 +1,7 @@
 # hubValidations (development version)

+* Introduced `hub_path` as main argument to `submission_tmpl()` and deprecated arguments `hub_con` and `config_tasks` (#165 & #137). This way, all that is required by the user to create a submission template is the path to a hub directory.


Suggested change

* Introduced `hub_path` as main argument to `submission_tmpl()` and deprecated arguments `hub_con` and `config_tasks` (#165 & #137). This way, all that is required by the user to create a submission template is the path to a hub directory.

* Introduced `path` as main argument to `submission_tmpl()` and deprecated arguments `hub_con` and `config_tasks` (#165 & #137). This way, all that is required by the user to create a submission template is the path to a hub directory or the config file.

R/submission_tmpl.R

zkamvar · 2025-02-03T16:36:16Z

tests/testthat/_snaps/submission_tmpl.md

-       7 2022-12-26    wk ahead inc~       2 US       sample      1                 NA
-       8 2022-12-26    wk ahead inc~       2 01       sample      1                 NA
-       9 2022-12-26    wk ahead inc~       2 02       sample      1                 NA
-      10 2022-12-26    wk ahead inc~       1 US       sample      2                 NA
+       7 2022-12-26    wk ahead inc~       2 US       sample      s1                NA
+       8 2022-12-26    wk ahead inc~       1 US       sample      s2                NA
+       9 2022-12-26    wk ahead inc~       2 01       sample      s3                NA
+      10 2022-12-26    wk ahead inc~       1 01       sample      s4                NA


I was initially confused why this section of the output had changed, but I now realize that it's because this snapshot test was originally in the section above where config_tasks was from a hub with a compound task ID structure

It's because the checks have been re-organised, some removed hence some new snapshots have been made

zkamvar · 2025-02-03T16:44:35Z

tests/testthat/_snaps/submission_tmpl.md

-       8 2022-12-26    wk ahead inc~       1 US       sample      1                 NA
-       9 2022-12-26    wk ahead inc~       2 01       sample      1                 NA
-      10 2022-12-26    wk ahead inc~       1 01       sample      1                 NA
+       8 2022-12-26    wk ahead inc~       2 01       sample      1                 NA
+       9 2022-12-26    wk ahead inc~       2 02       sample      1                 NA
+      10 2022-12-26    wk ahead inc~       1 US       sample      2                 NA


Same note as above, I believe this is an artifact of this check belonging to an old value for the config.

zkamvar · 2025-02-03T16:45:03Z

tests/testthat/_snaps/submission_tmpl.md

+       8 2022-12-26    wk ahead inc~       1 US       sample      1                 NA
+       9 2022-12-26    wk ahead inc~       2 01       sample      1                 NA
+      10 2022-12-26    wk ahead inc~       1 01       sample      1                 NA


Same note as above

zkamvar · 2025-02-03T16:45:44Z

tests/testthat/_snaps/submission_tmpl.md

+       8 2022-12-26    wk ahead inc~       1 US       sample      2                 NA
+       9 2022-12-26    wk ahead inc~       2 01       sample      3                 NA
+      10 2022-12-26    wk ahead inc~       1 01       sample      4                 NA


I believe this is the end of the modifications to the expected outputs

annakrystalli · 2025-02-04T11:58:58Z

That's a good question, if they have an internet connection, they could do something like this and get the file since jsonlite::fromJSON() understands URLs

cfg <- "https://raw.githubusercontent.com/reichlab/variant-nowcast-hub/refs/heads/main/hub-config/tasks.json"
submission_tmpl(path = cfg, ...)

This won't work unfortunately with the current setup:

library(hubValidations)
path <- "https://raw.githubusercontent.com/reichlab/variant-nowcast-hub/refs/heads/main/hub-config/tasks.json"
submission_tmpl(
  path,
  round_id = "2025-01-25"
  )
#> Error in `submission_tmpl()`:
#> ! `path`
#>   'https://raw.githubusercontent.com/reichlab/variant-nowcast-hub/refs/heads/main/hub-config/tasks.json'
#>   does not exist.

^{Created on 2025-02-04 with reprex v2.1.0}

zkamvar

This won't work unfortunately with the current setup:

Oh yeah, that's a good point. The fs::file_exists() catches that and it doesn't know about URLs :/

annakrystalli · 2025-02-05T14:58:24Z

Just a heads up @zkamvar

Given the changes proposed in hubverse-org/hubUtils#210 which will solve a couple of the outstanding issues here, I'm going to wait till we merge that, then adapt and re-request review. 🚀

… submission_tmpl() path arg.

annakrystalli · 2025-02-06T14:30:28Z

Ready for review @zkamvar !

zkamvar

Thank you! I have some requested changes for the news and to simplify the control structure for the path argument.

NEWS.md

zkamvar · 2025-02-06T15:37:59Z

R/submission_tmpl.R

+      if (inherits(path, "SubTreeFileSystem")) {
+        if (hubUtils::is_s3_base_fs(path)) {
+          return(read_config(path))
+        }
+        return(read_config_file(path))
+      }
+      # Read config file from URL
+      if (hubUtils::is_url(path)) {
+        if (hubUtils::is_github_repo_url(path)) {
+          return(read_config(path))
+        }
+        return(read_config_file(path))
+      }
+      # Read local config
+      if (!fs::file_exists(path)) {
+        cli::cli_abort("{.arg path} {.file {path}} does not exist.",
+          call = rlang::caller_env(1)
+        )
+      }
+      if (fs::is_dir(path)) {
+        return(read_config(path))
+      }
+      read_config_file(path)


Since much of the heavy lifting is done inside read_config() and read_config_file(), the main task is to determine if we have a directory or a file. This can be achieved with two indicator variables.

Suggested change

if (inherits(path, "SubTreeFileSystem")) {

if (hubUtils::is_s3_base_fs(path)) {

return(read_config(path))

}

return(read_config_file(path))

}

# Read config file from URL

if (hubUtils::is_url(path)) {

if (hubUtils::is_github_repo_url(path)) {

return(read_config(path))

}

return(read_config_file(path))

}

# Read local config

if (!fs::file_exists(path)) {

cli::cli_abort("{.arg path} {.file {path}} does not exist.",

call = rlang::caller_env(1)

)

}

if (fs::is_dir(path)) {

return(read_config(path))

}

read_config_file(path)

is_s3_dir <- inherits(path, "SubTreeFileSystem") && hubUtils::is_s3_base_fs(path)

is_local_dir <- is.character(path) && fs::path_ext(path) == ""

if (is_s3_dir || is_local_dir) {

read_config(path)

} else {

read_config_file(path)

}

❗ This will not pass tests until hubverse-org/hubUtils#211 is merged

Nice simplification! There are however exceptions that this wasn't handling well. For example, if someone erroneously supplied "https://github.com/hubverse-org/schemas/tree/main/v5.0.0" as a path, it was identified as a file and read_config_file() was invoked. That's why I added an additional check which catches that and returns a more informative error message.

is.character(path) && fs::path_ext(path) == "" is still a bit hacky as if a URL like "https://raw.githubusercontent.com/hubverse-org/example-simple-forecast-hub/refs/heads/main/README" or "https://raw.githubusercontent.com/hubverse-org/example-simple-forecast-hub/refs/heads/main/LICENSE" is supplied, it would trigger read_config() erroneously. Unfortunately the only method I found for ensuring the correct read function is deployed requires making a request for headers and checking the Content-Type to see if it’s a file. I've refrained from adding such a function but have added a test for that exception. It would be nice to handle better at some point (now it just fails because it interprets the URL as a folder and creates a path to tasks.json that does not exists).

Nice simplification! There are however exceptions that this wasn't handling well. For example, if someone erroneously supplied "https://github.com/hubverse-org/schemas/tree/main/v5.0.0" as a path, it was identified as a file and read_config_file() was invoked. That's why I added an additional check which catches that and returns a more informative error message.

is.character(path) && fs::path_ext(path) == "" is still a bit hacky as if a URL like "https://raw.githubusercontent.com/hubverse-org/example-simple-forecast-hub/refs/heads/main/README" or "https://raw.githubusercontent.com/hubverse-org/example-simple-forecast-hub/refs/heads/main/LICENSE" is supplied, it would trigger read_config() erroneously.

These are fair points and I definitely agree that the directory check is hacky, but I take solace in the hope that these situations are vanishingly rare and are easily fixed.

zkamvar

LGTM!

annakrystalli added 4 commits January 29, 2025 15:58

Deprecate hub_con and config_tasks arg in submission_tmpl in favour o…

d950730

…f hub_path. Resolves #165 and #137

Update tests to reflect deprecation of submission_tmpl args

8155a2d

Reorganise examples

cff0bfc

Update NEWS

41ce2da

annakrystalli requested a review from zkamvar January 29, 2025 15:27

Appease lintr

f0f90a8

annakrystalli mentioned this pull request Jan 29, 2025

Remove hub_con and config_tasks as submission_tmpl() args #197

Open

zkamvar requested changes Jan 29, 2025

View reviewed changes

R/submission_tmpl.R Outdated Show resolved Hide resolved

annakrystalli added 6 commits January 31, 2025 11:10

Change hub_path arg to path and allow for path to config file to be p…

bc4c048

…rovided

Update submission_tmpl examples

ef53dc9

Remove mention of hub connection "config_tasks" attribute from modell…

0727235

…ing task message.

Document

39828ba

Reorganise tests, add more specific tests.

3c7d9e8

remove duplicate check on whether path file/directory exists

68d4e41

Appease the lintr!

8f3e750

annakrystalli requested a review from zkamvar January 31, 2025 15:51

zkamvar requested changes Feb 3, 2025

View reviewed changes

Update NEWS

4467ee6

annakrystalli changed the title ~~Introduce hub_path and deprecate hub_con and config_tasks as args to submission_tmpl~~ Introduce path and deprecate hub_con and config_tasks as args to submission_tmpl Feb 4, 2025

Add caller environment to switch_get_config error msg

338b92b

Add comment

c35227d

annakrystalli mentioned this pull request Feb 4, 2025

Allow read_config_file() and read_config() to take URLs as inputs and add SubTreeFileSystem class method to read_config_file() hubverse-org/hubUtils#209

Closed

zkamvar self-requested a review February 4, 2025 17:06

zkamvar approved these changes Feb 4, 2025

View reviewed changes

annakrystalli mentioned this pull request Feb 5, 2025

Allow URLs and SubTreeFileSystem objects as inputs to read_config and read_config_file hubverse-org/hubUtils#210

Merged

Add support for URLs and S3 SubTreeFleSystem objects as inputs to the…

f23c09f

… submission_tmpl() path arg.

annakrystalli requested a review from zkamvar February 6, 2025 14:30

Fix example error

68bae8c

zkamvar requested changes Feb 6, 2025

View reviewed changes

annakrystalli and others added 6 commits February 10, 2025 11:46

Update NEWS

2a8c3a8

Simplify hub vs file switch while catching invalid GitHub URLs

0aa7e99

Update check to point to an existing file that is interpretted as folder

461cc5d

Add TODO note in test

05b61e6

typos

8805841

test commit to test verification

1295c9c

annakrystalli requested a review from zkamvar February 10, 2025 15:51

zkamvar approved these changes Feb 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce path and deprecate hub_con and config_tasks as args to submission_tmpl #196

Introduce path and deprecate hub_con and config_tasks as args to submission_tmpl #196

annakrystalli commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025 •

edited

Loading

zkamvar left a comment

annakrystalli commented Jan 31, 2025

zkamvar left a comment •

edited

Loading

zkamvar Feb 3, 2025

zkamvar Feb 3, 2025

annakrystalli Feb 4, 2025

zkamvar Feb 3, 2025

zkamvar Feb 3, 2025

zkamvar Feb 3, 2025

annakrystalli commented Feb 4, 2025

zkamvar left a comment

annakrystalli commented Feb 5, 2025

annakrystalli commented Feb 6, 2025

zkamvar left a comment

zkamvar Feb 6, 2025

annakrystalli Feb 10, 2025 •

edited

Loading

zkamvar Feb 10, 2025

zkamvar left a comment

		@@ -1,5 +1,7 @@
		# hubValidations (development version)

		* Introduced `hub_path` as main argument to `submission_tmpl()` and deprecated arguments `hub_con` and `config_tasks` (#165 & #137). This way, all that is required by the user to create a submission template is the path to a hub directory.

	* Introduced `hub_path` as main argument to `submission_tmpl()` and deprecated arguments `hub_con` and `config_tasks` (#165 & #137). This way, all that is required by the user to create a submission template is the path to a hub directory.
	* Introduced `path` as main argument to `submission_tmpl()` and deprecated arguments `hub_con` and `config_tasks` (#165 & #137). This way, all that is required by the user to create a submission template is the path to a hub directory or the config file.

Introduce path and deprecate hub_con and config_tasks as args to submission_tmpl #196

Are you sure you want to change the base?

Introduce path and deprecate hub_con and config_tasks as args to submission_tmpl #196

Conversation

annakrystalli commented Jan 29, 2025 • edited Loading

github-actions bot commented Jan 29, 2025 • edited Loading

zkamvar left a comment

Choose a reason for hiding this comment

annakrystalli commented Jan 31, 2025

zkamvar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annakrystalli commented Feb 4, 2025

zkamvar left a comment

Choose a reason for hiding this comment

annakrystalli commented Feb 5, 2025

annakrystalli commented Feb 6, 2025

zkamvar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annakrystalli Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkamvar left a comment

Choose a reason for hiding this comment

annakrystalli commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025 •

edited

Loading

zkamvar left a comment •

edited

Loading

annakrystalli Feb 10, 2025 •

edited

Loading