Type annotation improvements #19642
Type annotation improvements #19642
Conversation
github-actions
bot
added
area/testing
area/API
area/reports
area/database
| from galaxy.structured_app import BasicSharedApp | ||
| from galaxy.model.tool_shed_install import ToolShedRepository | ||
| from galaxy.util.path import StrPath | ||
| from tool_shed.structured_app import RequiredAppT |
There was a problem hiding this comment.
This is mostly all really great. This feels like a regression in the package structuring though. Nothing in Galaxy should depend on a tool_shed rooted package I think - I feel like I've worked really hard on that. We really want to spin the tool shed backend out and have the direction of dependency being the tool shed depends on Galaxy as a library and not the opposite. We've done so much work to unwind that - this feels like you're rewinding it back up in the wrong direction.
There was a problem hiding this comment.
Thanks for reviewing!
Of course I support the effort of splitting Galaxy in packages, having often contributed to it.
If you notice, the only places where new imports from the tool_shed module have been introduced in this PR are for type checking, which I believe should be treated separately.
I think it's OK to require the galaxy-tool-shed package when type checking galaxy ones, in the same way we e.g. require galaxy-app to type check galaxy-tool-util in
There are already many places where we do this, and I think it's an acceptable trade-off in order to have more precise type annotations.
There was a problem hiding this comment.
from galaxy.tools import Tool
This is a design bug IMO - we shouldn't be doubling down on it. The whole point of introducing tool-util was to decouple the parsing of tools from the Tool object. I would have asked Marius to not introduce that if I would have noticed it and I will try to find some time to unwind it.
There are already many places where we do this, and I think it's an acceptable trade-off in order to have more precise type annotations.
I'm not going to make any concrete statement one way or the other - I mostly disagree with this trade-off but there are going to be exceptions for years to come I think where it will make sense to make this trade off. In this case however - we're explicitly trying to decouple these things and that abstraction was introduced so that we wouldn't have dependencies (typing or otherwise) on tool shed code in Galaxy.
We had an interface for RequiredAppT in the correct spot (?) I think and you've moved it as part of this pull request. Can you explain why in a comment and add a TODO to move it back when the package structure is improved. Is there work on this component I can do to reorganize things so those runtime dependencies match typing dependencies?
There was a problem hiding this comment.
xref on why Tool is required at runtime and why I think it is inappropriate - https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/tool_util/parser/xml.py#L577.
|
Test failures unrelated. |
nsoranzo
force-pushed
the
type_annot
branch
3 times, most recently
from
9ee8df1 to
20ddd49
Compare
Also:
currentattribute ofscoped_sessionRequireAppTtypeHow to test the changes?
(Select all options that apply)
License