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

FlyteRemote data context does not get used #993

Merged
merged 12 commits into from
May 12, 2022
Merged

Conversation

wild-endeavor
Copy link
Contributor

@wild-endeavor wild-endeavor commented May 7, 2022

TL;DR

@cosmicBboy found this issue when working on the unionml library. To summarize,

Even though the FlyteRemote object creates a FlyteContext, when you use that remote object to fetch an execution, there is nothing tying that LiteralsResolver instance back to that context. So when you try to download/upload anything, you end up using the default FileAccessProvider created at the module layer.

To remediate, we can

  • Add a context to the LiteralsResolver and cache the remote object's context when we get execution inputs/outputs.
    • Is this the only object that will need access to the remote object's context? Is there anything else the user might interact with that would also require it?
  • More golang style context management - allow ctx to be specified in the get function (but then the user has to manually use it at call-time).
  • Push the context from the FlyteRemote onto the global stack in the constructor. This way anything that uses the context will see it. Some issues,
    • What if you create more than one remote object with different data configs?
    • What if another object, or the user manually, pushes additional FlyteContext objects onto the global stack? We do this already when we run workflows locally (inside the workflow/task __call__ function). Granted using a prior execution's input/outputs in a workflow declaration would be quite weird, but still something to consider.
  • Do the above but lock the context somehow.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Doing option 1 - it's the least amount of code.

Tracking Issue

https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
@wild-endeavor wild-endeavor changed the title FlyteRemote not setting context FlyteRemote data context does not get used May 9, 2022
@@ -145,7 +145,7 @@ def __init__(
)

# Save the file access object locally, but also make it available for use from the context.
FlyteContextManager.with_context(
FlyteContextManager.push_context(
Copy link
Contributor

Choose a reason for hiding this comment

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

:(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i don't like this either.

* save context into remote and literals resolver constructor

Signed-off-by: Yee Hing Tong <[email protected]>

* nits

Signed-off-by: Yee Hing Tong <[email protected]>
@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #993 (e4f68bc) into master (e2f6cdf) will increase coverage by 0.04%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #993      +/-   ##
==========================================
+ Coverage   86.29%   86.33%   +0.04%     
==========================================
  Files         252      255       +3     
  Lines       24258    24323      +65     
  Branches     2768     2768              
==========================================
+ Hits        20934    21000      +66     
  Misses       2851     2851              
+ Partials      473      472       -1     
Impacted Files Coverage Δ
flytekit/remote/remote.py 36.18% <57.14%> (+0.35%) ⬆️
flytekit/core/type_engine.py 84.96% <100.00%> (+0.01%) ⬆️
tests/flytekit/unit/core/test_literals_resolver.py 100.00% <100.00%> (ø)
tests/flytekit/unit/configuration/test_internal.py 100.00% <0.00%> (ø)
...kit/unit/core/functools/test_decorator_location.py 100.00% <0.00%> (ø)
...s/flytekit/unit/core/functools/decorator_source.py 63.63% <0.00%> (ø)
...ts/flytekit/unit/core/functools/decorator_usage.py 83.33% <0.00%> (ø)
flytekit/configuration/__init__.py 94.96% <0.00%> (+0.08%) ⬆️
flytekit/configuration/internal.py 95.77% <0.00%> (+0.12%) ⬆️
flytekit/clis/sdk_in_container/run.py 70.74% <0.00%> (+0.55%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2f6cdf...e4f68bc. Read the comment docs.

Signed-off-by: Yee Hing Tong <[email protected]>
@wild-endeavor wild-endeavor marked this pull request as ready for review May 10, 2022 16:27
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
eapolinario
eapolinario previously approved these changes May 11, 2022
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
eapolinario
eapolinario previously approved these changes May 12, 2022
Signed-off-by: Eduardo Apolinario <[email protected]>
@wild-endeavor wild-endeavor merged commit cc2a4e7 into master May 12, 2022
eapolinario pushed a commit that referenced this pull request Jun 17, 2022
@eapolinario eapolinario mentioned this pull request Jun 17, 2022
8 tasks
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