-
Notifications
You must be signed in to change notification settings - Fork 797
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
feature: add req parsers for configs #2248
base: master
Are you sure you want to change the base?
Conversation
np_deps: Dict[str, str], | ||
sys_deps: Dict[str, str], | ||
) -> Optional[str]: | ||
from packaging.requirements import InvalidRequirement, Requirement |
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.
only change to the original here, as packaging
is not available by default in environments, having the import trickle down to the top-level causes issues.
Let me make sure we properly override these internally and if so, this should be good to go. |
YML_SPLIT_LINE = re.compile(r"(?:=\s)?(<=|>=|~=|==|<|>|=)") | ||
|
||
|
||
def req_parser(config_value: str) -> Dict[str, Any]: |
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.
is it possible for extensions to define and override these parsers? that way this implementation can be decoupled with metaflow-netflixext
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.
There is no special mechanism right now. If this was something in core, the override we do for things like Parameter would work fine but here this is not. I don't think this will actually work right now (looking at it) because the plugin pypi
doesn't exist internally (it's not loaded). I'm going to have to play around with it a bit. We could of course add a similar plugin category PARSERS
(like everything else) and resolve it in the plugins. Bringing it back up here though will be a bit trickier.
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.
added a comment
moving from https://github.com/romain-intel/metaflow-dep-parsers
adds
req_parser
for@pypi
andyml_parser
for@conda
to enable passing dependencies as config values.