-
Notifications
You must be signed in to change notification settings - Fork 240
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
Pass in env vars in FlyteRemote register_script #2120
Pass in env vars in FlyteRemote register_script #2120
Conversation
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
…ter_script Signed-off-by: Karl Nordstrom <karl.n@muonspace.com>
Signed-off-by: Karl Nordstrom <karl.n@muonspace.com>
a3074f3
to
a4facf0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2120 +/- ##
=======================================
Coverage 85.78% 85.78%
=======================================
Files 313 313
Lines 23525 23525
Branches 3517 3517
=======================================
Hits 20182 20182
Misses 2734 2734
Partials 609 609 ☔ View full report in Codecov by Sentry. |
Congrats on merging your first pull request! 🎉 |
@knordstrom-muon Sorry, just a quick validation from myself, but this is an implementation only valid for "pyflyte register", but not using "flytectl register". Am I right? |
Hi @nineunderground -- I am not using this with
So I think in your case, you could achieve it with |
Why are the changes needed?
The
FlyteRemote.register_script
function is useful for registering local code to remote during integration tests of workflows. It does not allow the user to pass in environment variables to the serialization though, even though it should be easy to support this.What changes were proposed in this pull request?
Make it possible to pass in environment variables using a
dict[str,str]
which can be passed directly toSerializationSettings
.How was this patch tested?
Don't think
register_script
is covered by unit tests at the moment.