-
Notifications
You must be signed in to change notification settings - Fork 287
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
[Flyte Deck] Fix Lazy Import Error for Pandas and Plotly #2783
Conversation
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2783 +/- ##
==========================================
+ Coverage 83.85% 91.61% +7.76%
==========================================
Files 3 105 +102
Lines 161 5317 +5156
==========================================
+ Hits 135 4871 +4736
- Misses 26 446 +420 ☔ View full report in Codecov by Sentry. |
@@ -96,6 +94,8 @@ def __init__(self, column_name): | |||
self._column_name = column_name | |||
|
|||
def to_html(self, df: "pd.DataFrame") -> str: | |||
import plotly.express as px |
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.
This means we need plotly as a hard dependency on the deck plugin, right?
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.
nope, this means if we import BoxRenderer
and use it in our task, this will be a required dependency
Thanks @Future-Outlier 👍 |
* [Flyte Deck] Fix Lazy Import Error for Pandas and Ploty Signed-off-by: Future-Outlier <[email protected]> * update fix Signed-off-by: Future-Outlier <[email protected]> --------- Signed-off-by: Future-Outlier <[email protected]>
Tracking issue
flyteorg/flyte#5792
Why are the changes needed?
if we install
flytekitplugins-deck-standard
andflytekit
, when importflyetkit
, it will raise an error aboutModuleNotFoundError
orImportError
What changes were proposed in this pull request?
pandas
inpd.DataFrame
as type hint.import plotly.express as px
in the functionto_html
for each class instead of using lazy import, since lazy import will require user to installplotly
.How was this patch tested?
Dependency Check
Deck Example
Setup process
local execution and remote execution.
Screenshots
Check all the applicable boxes
Related PRs
#2766