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

Forbid attribute_filters={"key": None} case #2186

Closed
tcompa opened this issue Jan 16, 2025 · 3 comments · Fixed by #2202
Closed

Forbid attribute_filters={"key": None} case #2186

tcompa opened this issue Jan 16, 2025 · 3 comments · Fixed by #2202
Assignees
Labels
API flexibility Support more workflow-execution use cases

Comments

@tcompa
Copy link
Collaborator

tcompa commented Jan 16, 2025

Second

attribute_filters={"key1": None, "key2": ["A", "B"]}
-> require that image has attribute key2 equal to A or B, and do not apply any filtering for key1

This is due to:

  • Remaining backwards-compatible (since up to v2.10 setting an attribute value to None would disable that filter).
  • The fact that we often had to merge different sets of filters (not any more, see Replace attribute filters rather than merging them #2155) and then this was way to represent a patch that would disable a previously set filter.

The latter reason does not hold any more, and I guess the first one is not very relevant in real-life usage. Thus I'd be in favor of deprecating this option. Either way, we'll have to review this (because it's currently not fully backwards compatible, in the API schemas).
Originally posted by @tcompa in #2182

@tcompa tcompa added the flexibility Support more workflow-execution use cases label Jan 16, 2025
@tcompa
Copy link
Collaborator Author

tcompa commented Jan 21, 2025

@tcompa Agreed to all! Looks good to me. I'll review additional type filter questions on that issue separately

attribute_filters={"key1": None, "key2": ["A", "B"]}
...
Thus I'd be in favor of deprecating this option.

For job attribute filters, we can remove this without an issue. Given that the only source for defaults for job attribute filters are the dataset attribute filters, I don't see a use for keeping the support for None = unset the filter anymore as well.
Agreed on deprecating it.

Originally posted by @jluethi in #2182

@tcompa tcompa changed the title Review attribute_filters={"key": None} case Forbid attribute_filters={"key": None} case Jan 21, 2025
@tcompa
Copy link
Collaborator Author

tcompa commented Jan 21, 2025

  • Update our custom type for attribute filters
  • Double check whether we need to explicitly forbid this case in some fractal_server.image or API module
  • Update data-migration script, so that any occurrence of this "key": None pair is removed, with a warning.

@tcompa
Copy link
Collaborator Author

tcompa commented Jan 28, 2025

Closed with #2202

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API flexibility Support more workflow-execution use cases
Projects
Development

Successfully merging a pull request may close this issue.

2 participants