Skip to content
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

Plugins lazy module loading #2049

Merged
merged 7 commits into from
Dec 18, 2023
Merged

Plugins lazy module loading #2049

merged 7 commits into from
Dec 18, 2023

Conversation

cosmicBboy
Copy link
Contributor

Tracking issue

Fixes flyteorg/flyte#4401

Why are the changes needed?

This change is primarily to simplify the dependencies needed for things like generating API reference documentation for flytekit. Before one needed all dependencies of all plugins to render API docs.

What changes were proposed in this pull request?

Use flytekit.lazy_module to lazily load modules in the plugin packages.

How was this patch tested?

Run tests locally with pytest

Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com>
Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com>
Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com>
Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com>
Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com>
Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com>
Signed-off-by: Niels Bantilan <niels.bantilan@gmail.com>
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (530ad26) 83.18% compared to head (df94f17) 85.98%.
Report is 4 commits behind head on master.

Files Patch % Lines
...ins/flytekit-modin/flytekitplugins/modin/schema.py 78.57% 3 Missing ⚠️
...s/flytekit-airflow/flytekitplugins/airflow/task.py 85.71% 2 Missing ⚠️
...tekit-snowflake/flytekitplugins/snowflake/agent.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2049      +/-   ##
==========================================
+ Coverage   83.18%   85.98%   +2.79%     
==========================================
  Files         173      308     +135     
  Lines       16840    22940    +6100     
  Branches     3471     3468       -3     
==========================================
+ Hits        14009    19725    +5716     
- Misses       2236     2615     +379     
- Partials      595      600       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pingsutw pingsutw assigned pingsutw and cosmicBboy and unassigned pingsutw Dec 14, 2023
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had heard stories about lazy importing breaking some workflows. For example in flytekit: flyteorg/flyte#3853 .

Overall, I am okay with making plugin imports lazy. Moving forward, how do we make sure plugins always lazy import dependencies? I suspect the monodocs will fail with an "can not import ___" error, which does not directly connect to the solution of lazy loading.

@pingsutw
Copy link
Member

I had heard stories about lazy importing breaking some workflows. For example in flytekit: flyteorg/flyte#3853 .

I also fixed that in this PR

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I also tested the airflow plugin, and it works well for me.

@cosmicBboy
Copy link
Contributor Author

cosmicBboy commented Dec 18, 2023

Overall, I am okay with making plugin imports lazy. Moving forward, how do we make sure plugins always lazy import dependencies? I suspect the monodocs will fail with an "can not import ___" error, which does not directly connect to the solution of lazy loading.

Short of catching this in a review (or monodocs build thing), not sure how we can handle this.

Edit: I guess we can also add a plugin unit test in CI:

  • Installs flytekit in the environment
  • It should be able to import the plugin with only flytekit and its dependencies
  • If not, the test should fail with an import error.

@cosmicBboy
Copy link
Contributor Author

Created this issue @thomasjpfan: we can discuss the plugin convention issue there - flyteorg/flyte#4619

@cosmicBboy cosmicBboy merged commit 8fd3dfa into master Dec 18, 2023
75 checks passed
@eapolinario
Copy link
Collaborator

Lazy-loading pyspark.ml causes issues on executions of Spark tasks on real clusters. More details in #2184. Reverting just that particular line works, but I'm not sure what the repercussions of this to monodocs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants