Save more provenance information #308
Conversation
|
Quickly skimmed the PR (will do a more thorough look later on), and so far it looks good. Would there be a situation where the app isn't a distribution package installed in the environment? |
An example would be if I find a Snakebids app I want to use on GitHub, clone it, and have Snakebids etc. globally installed in my environment, so I don't bother installing the app to my environment. A real-world (if kind of weird and likely to change) case is https://github.com/afids/afids-CNN/tree/main/afids_cnn, which wraps a couple of different Snakebids workflows under one package that is not itself a Snakebids workflow. |
|
Fair enough, then I think one useful thing to add would be a warning that "...the version of the snakebids app cannot be found as it is likely not installed in the environment" or something to that effect, so its not a surprise that the config shows "Unknown" version. |
snakebids/app.py
Outdated
| @@ -240,6 +250,21 @@ def create_descriptor(self, out_file: PathLike[str] | str) -> None: | |||
| ) | |||
| new_descriptor.save(out_file) # type: ignore | |||
|
|
|||
| def get_app_version(self) -> str | None: | |||
| """Attempt to get the app version, returning None if we can't. | |||
There was a problem hiding this comment.
If we're moving in this direction, I wonder if we should allow (and encourage) the passing of explicit modules (or string names of modules) to snakemake_dir in addition to paths. This would actually simplify the run.py file a bit. And this would create some extra liberty e.g. if the snakemake package was in some subpackage foo.bar, we'd be able to support that a little better.
There was a problem hiding this comment.
Not necessarily a goal for this PR, but maybe something to think about for the future
There was a problem hiding this comment.
I didn't address this for now, but is probably worth a separate issue for discussion.
There was a problem hiding this comment.
One concern with leaving a warning if no version is found is that if a developer messes up the implementation preventing versioning, users will get that warning and potentially think something is wrong... But then they may still think something is wrong when they read version=unknown so maybe it doesn't matter.
|
I added a warning where I tried to convey that the missing version will often be nothing for the end user to worry about, but I'm open to changing either or both of the warning's text, and its log level (could make it |
|
@tkkuehn, we had originally said we'd try to get this one in our 0.9 release. Would you still like to? If not, there's no need to maintain py37 support |
I'm open either way, I don't think this is such a major feature that we need to get it in ASAP, and delaying reduces the amount of work to be done to drop py37 I guess |
|
How about we just do a race. If the py37 drop is done before this, I'll release without this, otherwise this can go in |
I think this is currently ready to go, unless you have any more comments? |
|
Nope, looks like just a merge, then you can go ahead |
Proposed changes
Addresses #205 in a reasonably basic way. It's not guaranteed that a Snakebids app even has a version, so this PR just takes a crack at it by assuming the name of the snakemake directory is also the name of an installed package. If anyone has any other ideas, we can straightforwardly add fallback options to
SnakeBidsApp.get_app_version.Note that this doesn't do anything about the proposed formatting/comments in the output config. Maybe that would be good to raise as a separate issue.
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you are unsure about any of the choices, don't hesitate to ask!Notes
All PRs will undergo the unit testing before being reviewed. You may be requested to explain or make additional changes before the PR is accepted.