-
Notifications
You must be signed in to change notification settings - Fork 18
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
quality of life improvements related to importing mmda types, predictors, etc. #141
Conversation
from mmda.types|predictors|parsers import *
possibleimport PysbdSentenceBoundaryPredictor | ||
__all__.append('PysbdSentenceBoundaryPredictor') | ||
|
||
with necessary(["layoutparser", "torch", "torchvision", "effdet"], soft=True) as PYTORCH_AVAILABLE: |
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.
It would be nice to not have to define this in three places:
- setup extras_require
- required_backends on the predictor itself
- here
But I don't have a good suggestion.
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.
yeah :/ ive created an issue here: #144
i need to think a bit more how this would be resolved. a reasonable stop-gap could be defining a top-level config that all 3 locations imports from; reserve for next PR
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.
I think this all makes sense. You'll want to bump setup.py version +0.0.1 again because I think @geli-gel beat you to the punch with v0.0.36.
Putting the api types into ai2_internal also seems like a good move but note it will be breaking for the parsers/models still operationalized in spp instead of TIMO. Should be relatively easy to fix if/when we upgrade them though, I think.
from mmda.types|predictors|parsers import *
possible