Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add/update_sample needs to validate --map_strategy #54

Closed
marvin-jens opened this issue Dec 1, 2022 · 3 comments · Fixed by #94
Closed

add/update_sample needs to validate --map_strategy #54

marvin-jens opened this issue Dec 1, 2022 · 3 comments · Fixed by #94
Assignees
Labels
enhancement New feature or request

Comments

@marvin-jens
Copy link
Member

Currently, no validation of the map_strategy string is performed when adding or updating a sample. If a reference is used that does not exist in the config.yaml, this triggers a confusing KeyError in mapping.smk during spacemake run.

Expected behavior: An exception should be thrown when adding or updating a sample with an invalid map_strategy.

@marvin-jens marvin-jens self-assigned this Dec 1, 2022
@danilexn danilexn added the enhancement New feature or request label Jan 15, 2024
@nukappa
Copy link
Member

nukappa commented Jan 18, 2024

We could use the ConfigVariableNotFoundError for this. Currently map_strategy is the only (important) project variable not defined in the config.yaml. How about we ship spacemake with a few default ones and provide cmd line support to add/update more strategies? It'll make it easier to catch errors, give a default to the user etc

@danilexn danilexn mentioned this issue Jan 20, 2024
3 tasks
@danilexn danilexn linked a pull request Jan 20, 2024 that will close this issue
3 tasks
@marvin-jens
Copy link
Member Author

marvin-jens commented Jan 26, 2024

Hi Daniel, just to let you know, during fixing #88 on fast-cmdline I've noticed that your commits introduced errors. ConfigVariableNotFoundError expected not just a string with the error message but also the name of the variable that was not found. Then, the way you used map_strategy.validate_mapstr, you turned a string into a list with a single string in it (corrected_map_strategies.append(...)). This caused breakage downstream. Let's all please make sure that pytest -m "not big_download" tests/ runs through w/o issues before opening a PR. I should have checked that the tests finish, but when I tried today - after the merge - I found additional issues with the tests when executed anywhere else than my home directory on margaret. So joke's on me, really. :) However, with recent fast-cmdline all tests should now run anywhere (just tested on my laptop w/o /data/rajewsky ...).

@marvin-jens
Copy link
Member Author

This being said, I kept your changes functionally intact. Will now create a branch to add a cmdline test to ensure that add/update_sample indeed validates and then close the issue as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants