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

pybids_db_dir was eliminated without warning, causing migration issues #333

Closed
pvandyken opened this issue Aug 24, 2023 · 2 comments · Fixed by #334
Closed

pybids_db_dir was eliminated without warning, causing migration issues #333

pvandyken opened this issue Aug 24, 2023 · 2 comments · Fixed by #334

Comments

@pvandyken
Copy link
Contributor

In #296, we consolidated all the database related terms into one single name: pybidsdb_dir. We added a helpful deprecation warning to the old pybids_database_path parameter in generate_inputs, however, we failed to implement any deprecation cycle for the config entry: pybids_db_dir. If you run a workflow with snakebids>=0.9, that entry will not be set.

In all of my workflows, I had a line in my generate_inputs call:

pybids_database_dir=config.get("pybids_db_dir")

After upgrading, snakebids would silently fail to read my database (since it was just getting None to pybids_database_dir). This has stymied me a couple of times already, and I was one of the people behind the rename!

I think it would be really good to have some sort of deprecation, but unfortunately it will be a bit tricky. In the pattern above, we really can't tell if the user is selecting pybids_db_dir out of the config dict. The only way I thought would be to set that variable to some magic value which we can detect. It feels brittle, but at least it would be a temporary measure until we fully deprecate the parameter. And it's arguably better than the current situation.

Any thoughts on this or any better ideas? @tkkuehn @kaitj. If I make a change I'll push it onto the 0.9.x branch so that people have a chance of finding it.

@tkkuehn
Copy link
Contributor

tkkuehn commented Aug 24, 2023

Oh yeah, this is unfortunate. The only symptom of this we can detect is that generate_inputs is called with pybids_database_path=None, which can legitimately happen if there's actually no database. Without a way to inspect the config dict during a snakemake run, I don't think there's a better way to raise a deprecation warning, so I'm onboard to set it to some sentinel value.

@kaitj
Copy link
Contributor

kaitj commented Aug 24, 2023

I am also onboard. I think the plan was to deprecate on the next cycle anyways.

pvandyken added a commit to pvandyken/snakebids that referenced this issue Aug 24, 2023
Pybidsdb values encoded in the config file had their key names changed
without warning in snakebids==0.9.0. This caused apps relying on those
configuration names to silently fail to read the database

With this fix, once again set the keys with parsable sentinal values so
that reliant workflows will still work but maintainers will be alerted
to the deprecation

Resolves khanlab#333
@pvandyken pvandyken linked a pull request Aug 24, 2023 that will close this issue
pvandyken added a commit to pvandyken/snakebids that referenced this issue Aug 24, 2023
Pybidsdb values encoded in the config file had their key names changed
without warning in snakebids==0.9.0. This caused apps relying on those
configuration names to silently fail to read the database

With this fix, once again set the keys with parsable sentinal values so
that reliant workflows will still work but maintainers will be alerted
to the deprecation

Resolves khanlab#333
pvandyken added a commit to pvandyken/snakebids that referenced this issue Sep 8, 2023
Pybidsdb values encoded in the config file had their key names changed
without warning in snakebids==0.9.0. This caused apps relying on those
configuration names to silently fail to read the database

With this fix, once again set the keys with parsable sentinal values so
that reliant workflows will still work but maintainers will be alerted
to the deprecation

Resolves khanlab#333
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 a pull request may close this issue.

3 participants