Conversation
Member
Fair and probably an oversight
Makes sense to define our own runtime exception if needed |
denis-yuen
approved these changes
Apr 29, 2025
kathy-t
approved these changes
Apr 30, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR updates the snakemake plugin so that the initial path must point at a
Snakefilefor the resulting version to be valid. It also changes theinitialPathPatternto match any absolute path that ends with/Snakefileor/snakefile(see the documentation for the--snakefileoption for why we would support the lowercase name https://snakemake.readthedocs.io/en/stable/executing/cli.html).The goal is to prevent any valid Snakemake versions from having a primary descriptor that isn't the Snakefile.
Some notes and observations:
The plugin
initialPathPatternmethod does not appear to be used anywhere in our codebase, other than to output some diagnostic information. One might expect our workflow registration code to confirm that the initial path matches the pattern, but it does not. Essentially, this PR implements this type of check in the Snakemake plugin, only, to avoid breaking existing workflows of other types that might fail such a test.Currently, there is no defined way for a plugin to intentionally signal an exceptional condition to the surrounding code, in a way that the surrounding code can tell the difference between the intentional exception and and some other type of
RuntimeExceptionthat resulted from a bug. Such a mechanism would be useful in cases that the plugin purposefully wanted to "throw up its hands", stop, and propagate a reason to the surrounding code.The
FileReaderinterface is functionally very similar to theFileTreeabstraction introduced by the inference code. It is my belief that, conceptually, they are analogs.If we're serious about using the plugin architecture to support more workflow types, we should assess its architecture to make sure it is flexible enough to field all of the possible workflow types we might throw at it. Sooner we do this, the less plugins we'll need to rewrite.
Our registration code has some pretty deep spaghetti in spots, through which the repo access code is interleaved. At some point, when we have the time, it would be great to refactor. It would take a while, but make the code much easier to understand and maintain, and give us the ability to do some interesting new stuff.