metaflow
metaflow copied to clipboard
Netflix
Parse environment variables passed from CLI
Right now if one attempted to pass environment variables to the @environment decorator using the following type of syntax:
python workflow/myflow.py --with environment:vars=FOO:$(FOO),BAR:$(BAR)
vars would incorrectly register as a string rather than parsing the the contents as a dict.
This PR enables parsing of environment for the environment decorator which are passed using --with
Nice catch! The only thing that gives me a bit of a pause here is that it is not uncommon to have : in env variable values (eg $PATH). On the other hand I don't have a good idea how to support those without making decospec syntax even more complicated.
Nice catch! The only thing that gives me a bit of a pause here is that it is not uncommon to have
:in env variable values (eg$PATH). On the other hand I don't have a good idea how to support those without making decospec syntax even more complicated.
Perhaps I can update this to only parse the first : in each comma delimited block
After testing this a bit more, this doesn't seem to work with --batch. In that case gets converted back to --with CLI arg here, and it turns it into --with 'environment:vars={'\"MY_FOO\"': '\"bar\"', '\"MY_BAR\"': '\"baz\"'}'. Let me take a deeper look, there gotta be a way around this
Something like this https://github.com/Netflix/metaflow/commit/d654bc8727b18f88c27e90bb95288a0f52ff8c3c should work but may still need a bit more testing
Something like this d654bc8 should work but may still need a bit more testing
This looks good, though probably still need to limit the split on : to 1. This implementation should make it so that the only disallowed character in env var values would be ,.
Something like this d654bc8 should work but may still need a bit more testing
This looks good, though probably still need to limit the split on
:to 1. This implementation should make it so that the only disallowed character in env var values would be,.
I've updated this to split on the first colon
Something like this d654bc8 should work but may still need a bit more testing
I've updated the PR to incorporate these changes, but I have not thoroughly tested this.
@oavdeev @LarsDu still needs some testing but i've made the changes I think would make this work for both step-functions and batch plugins.
@savingoyal had a suggestion that we could solve it in a slightly more elegant way by making @environment a "varargs" style decorator. So instead of @environment(vars={"FOO": "BAR"}) you could support @environment(FOO="BAR"). Metaflow already supports passing multiple parameters to a decorator from the command line, so it allows to implement this without making --with syntax special for @environment.
I've implemented it on top of this PR here https://github.com/Netflix/metaflow/commit/31519fe -- please take a look
