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

fix: remove raise that limits using --config with dicts #1341

Merged
merged 5 commits into from Feb 21, 2022

Conversation

lpla
Copy link
Contributor

@lpla lpla commented Jan 20, 2022

Description

Snakemake use through other Python scripts using its API class

>>> import snakemake
>>> snakemake.snakemake(snakefile='Snakefile', config={"this_option": "does_not_break", "test": {'this_dict':'will_break'}})

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/lpla/bitextorment/lib/python3.8/site-packages/snakemake/__init__.py", line 508, in snakemake
    config_args = unparse_config(config)
  File "/home/lpla/bitextorment/lib/python3.8/site-packages/snakemake/__init__.py", line 960, in unparse_config
    raise ValueError("config may only be a flat dict")
ValueError: config may only be a flat dict

allows setting any kind of parameters but dictionaries. The code this PR removes was triggering an exception when there is no issue in using dictionaries as a setting inside the Python dictionary given to the Snakemake class through config.

A simple test named test_dicts_in_config(), similar to the code above, is added to tests/testapi.py file to reproduce this issue. Test could be even simpler, as the Snakefile can be completely empty to reproduce the issue, but the one I wrote shows a rule using dicts through config working without the raise.

QC

  • The PR contains a test case for the changes or the changes are already covered by an existing test case.
  • The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

@lpla lpla requested a review from johanneskoester as a code owner Jan 20, 2022
@johanneskoester johanneskoester changed the title Remove raise that limits using --config with dicts feat: remove raise that limits using --config with dicts Jan 26, 2022
@johanneskoester
Copy link
Contributor

johanneskoester commented Jan 26, 2022

Thanks! Actually a test of the CLI is possible. This works via the shellcmd argument of the test run function, see this example: https://github.com/snakemake/snakemake/blob/main/tests/tests.py#L267

@lpla
Copy link
Contributor Author

lpla commented Jan 26, 2022

Hi. Sorry about the initial version of the pull request. I thought we were running Snakemake through a Bash script, but it was a Python one, so we were using the Snakemake API. I updated the description and added a simple test. Let me know if this style is correct, or if I could do anything else to fix it better or cleaner.

@lpla
Copy link
Contributor Author

lpla commented Jan 27, 2022

Also, I saw you edited the title as "feat:". Isn't this a bug?

@Pbdas
Copy link

Pbdas commented Feb 3, 2022

Just want to second this request. I ran into this limitation yesterday; as it is right now I'd have to choose between changing the config values I want to alter to be flat, or something janky like having a template config file in which I do string replacement. Passing a (nested) python dict is so much cleaner. Hope to see it in the next version so I can use it in production!

@Pbdas
Copy link

Pbdas commented Feb 3, 2022

Actually, there is a somewhat dirty hack to make this behaviour work correctly, by specifying some, possibly meaningless, dummy "config_args" value when calling snakemake from python:

snakemake.snakemake(
  ...
  # dict with just the values to be updated, properly nested as the config expects it
  config=config_overrrides,
  # some dummy config arg, as would normally be passed on the command line via --config-args
  config_args=["dummy=0"]
  )

This is because the throwing function unparse_config is only called if there is a config specified, but no config_args, see here

@lpla lpla changed the title feat: remove raise that limits using --config with dicts fix: remove raise that limits using --config with dicts Feb 15, 2022
@sonarcloud
Copy link

sonarcloud bot commented Feb 20, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@johanneskoester johanneskoester merged commit bd65057 into snakemake:main Feb 21, 2022
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants