-
Notifications
You must be signed in to change notification settings - Fork 244
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
Enable the flytekit to be pluggable #2039
Enable the flytekit to be pluggable #2039
Conversation
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2039 +/- ##
==========================================
- Coverage 85.27% 83.87% -1.40%
==========================================
Files 274 309 +35
Lines 21360 23060 +1700
Branches 3481 3489 +8
==========================================
+ Hits 18214 19342 +1128
- Misses 2537 3095 +558
- Partials 609 623 +14 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
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 moved the FlytekitPlugin
to flytekit.configuration
because it is "configuration flytekit".
@cosmicBboy The monodocs failure in the CI is unrelated to this PR, but looks real. |
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
In 9b90029 I have pytest patch the plugin variable. This way if another plugin gets loaded, the tests still run with the first-party |
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
I updated the solution to #2039, which I am happy with. This allows pytest to configure the loaded plugin on demand. |
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
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, could you resolve the merge conflict?
Alternative to #1941
Why are the changes needed?
This PR makes
pyflyte
pluggable by external libraries to adjust the CLI commands and update the remote.What changes were proposed in this pull request?
This PR adds a
FlytekitPlugin
to have the pluggable CLI API be in one place. For this initial PR, it gives the minimum API surface for configuring the API.Implementation-wise, the external plugins are loaded through entry points and have the group name:
flytekit.configuration.plugin
.How was this patch tested?