-
Notifications
You must be signed in to change notification settings - Fork 17
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
Reorganize imports #122
Reorganize imports #122
Conversation
bioimage.spec.latest.build_spec moved to bioimage.spec.build_spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I think it would be good if @FynnBe has another look at this too.
Edit: Sorry, I didn't see that this is still marked as draft, I assumed it was good to go since tests pass.
Thx @constantinpape for the review. I left it a draft because I felt it turned out a bit more complicated than I anticipated and was not sure whether it was a good idea or not. But I think we'd have similar issues with the custom finder/loader approach. Let's let @FynnBe have a look and then maybe decide. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
it would be great to add a CI check and maybe also a pre-commit hook for convenience.
bioimageio/spec/__init__.py
Outdated
# assuming schema will always be part of spec | ||
from . import schema | ||
from .raw_nodes import FormatVersion | ||
__version__ = schema.get_args(FormatVersion)[-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you are actually assuming that schema always imports typing.get_args
Did you avoid
try:
from typing import get_args
except ImportError:
from typing_extensions import get_args
her ein init.py on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. this doesn't make any sense - I'm going to move that import to spec.shared.common
, and change all imports accordingly.
# Conflicts: # bioimageio/spec/latest/__init__.py # bioimageio/spec/v0_3/schema.py # bioimageio/spec/v0_3/utils.py
Summary:
bioimageio.spec.latest
module.gen_spec
is moved tobioimageio.spec
.v0_3
. This makes thev0_3
spec available frombioimage.spec
.I kept this as draft, because after actually doing it, I think the approach with the script that generates those modules might be to fragile. Also I feel I would have to document it a lot, which also seems like a sign, that it's a bad idea. Maybe living with the fact that
from bioimageio.spec.raw_nodes import Something
doesn't work is less painful.fixes #120