[24.2] Fix various bugs around tool data table bundles #19659
Conversation
This is a little cleaner. Also replaces the `list[tuple[str, str, bool]]` return type of get_options with a NamedTuple. Fixes galaxyproject#19609
The file_size hack turns out to not work, since people report related bugs that they shouldn't have been able to run into without generating bundles. It's probably still true that bundles should produce a distinct datatype.
mvdbeek
force-pushed
the
fix_user_tables
branch
from
b6823fc to
be3db67
Compare
| @@ -66,6 +68,10 @@ | |||
| union_type, | |||
| ) | |||
|
|
|||
| if TYPE_CHECKING: | |||
| from galaxy.model import DatasetInstance | |||
| from galaxy.security.idencoding import IdEncodingHelper | |||
There was a problem hiding this comment.
Please don't break the dependency structures 😿. galaxy-data stuff doesn't belong in the galaxy-tool-util package. It seems like it would be really clean to just move ParameterOption into galaxy.tools.parameters somewhere - nothing else in the parameter model layer even seems to depend on it.
There was a problem hiding this comment.
I think this is fine, see #19642 (comment) for a rationale.
There was a problem hiding this comment.
It's fair, in a previous version I did actually need ParameterOption in tool_util. @jmchilton's comment also applies to line 46
There was a problem hiding this comment.
Yep, line 46 is not type checking, so that's really breaking the package dependency structure.
There was a problem hiding this comment.
The type imports are indicating a real dependency issue here - these are app concerns, they belong in app - the type dependency are evidence of it but it is true of them independently. I had no clue we were forward importing types but that seems like it is going to mask all sorts of problems we should be resolving with interfaces and better package structure. We really should not be doing that IMO and if it needed it should be used very minimally and we should document the problems they are indicating and create issues for follow up to resolve them.
There was a problem hiding this comment.
I really appreciate you moving the class @mvdbeek - thank you so much!
mvdbeek
force-pushed
the
fix_user_tables
branch
from
6924729 to
8c4149b
Compare
Fixes #19609 and #18749 and also cleans up what data manager artefacts are potential bundles.
How to test the changes?
(Select all options that apply)
License