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

try removing multiple imports to resolve fast-mode issue #2110

Merged
merged 2 commits into from
Jun 21, 2024

Conversation

novahow
Copy link
Contributor

@novahow novahow commented Jan 17, 2024

Tracking issue

flyteorg/flyte#4716

Why are the changes needed?

Please refer to may example in flyteorg/flyte#4716 (comment)

When we have a python file that contains workflows, tasks, and launch plans and try to run that in fast mode, we get an error:
AssertionError: The cached values aren't the same as the current call arguments

This happens because when executing the file, the launch_plan's get_or_create method is invoked by the following snippet multiple times, thus the LaunchPlan.CACHE stores a workflow. After that when we're really creating a plan with standard_scale_launch_plan = LaunchPlan.get_or_create(, the workflow of the plan is different from the workflow of the cached plan, which I haven't figured out the reason.

What changes were proposed in this pull request?

My workaround is removing exec_module in the try block and import_module in the exception block so that get_or_create won't be invoked multiple times

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

Copy link

welcome bot commented Jan 17, 2024

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.81%. Comparing base (29662e7) to head (f144c96).
Report is 1 commits behind head on master.

Current head f144c96 differs from pull request most recent head b7c138c

Please upload reports for the commit b7c138c to get more accurate results.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2110       +/-   ##
===========================================
+ Coverage   71.79%   85.81%   +14.01%     
===========================================
  Files         182      313      +131     
  Lines       18561    23503     +4942     
  Branches     3654     3511      -143     
===========================================
+ Hits        13326    20169     +6843     
+ Misses       4592     2726     -1866     
+ Partials      643      608       -35     

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

@novahow novahow changed the title try removing multiple mports to resolve fast-mode issue try removing multiple imports to resolve fast-mode issue Jan 17, 2024
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.

cc @cosmicBboy mind taking a look

@pingsutw pingsutw requested a review from samhita-alla as a code owner June 19, 2024 18:02
@pingsutw pingsutw merged commit 876877a into flyteorg:master Jun 21, 2024
45 of 46 checks passed
bgedik pushed a commit to bgedik/flytekit that referenced this pull request Jul 3, 2024
fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
Signed-off-by: novahow <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants