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

Swap out inspect file location #991

Merged
merged 9 commits into from
May 6, 2022
Merged

Swap out inspect file location #991

merged 9 commits into from
May 6, 2022

Conversation

wild-endeavor
Copy link
Contributor

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

TL;DR

If the decorator and the usage of your decorator are in different places, it seems the new logic will inadvertently use the module where the decorator is defined instead of where the task is defined.

Slack thread.

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

Since the module is being correctly set by functools, we should just use that to get the file path.

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

codecov bot commented May 5, 2022

Codecov Report

Merging #991 (1b0b1e0) into master (64ad510) will increase coverage by 0.00%.
The diff coverage is 82.14%.

@@           Coverage Diff            @@
##           master     #991    +/-   ##
========================================
  Coverage   86.28%   86.29%            
========================================
  Files         252      255     +3     
  Lines       24137    24284   +147     
  Branches     2747     2768    +21     
========================================
+ Hits        20826    20955   +129     
- Misses       2843     2856    +13     
- Partials      468      473     +5     
Impacted Files Coverage Δ
...s/flytekit/unit/core/functools/decorator_source.py 63.63% <63.63%> (ø)
...ts/flytekit/unit/core/functools/decorator_usage.py 83.33% <83.33%> (ø)
flytekit/core/tracker.py 78.89% <100.00%> (ø)
...kit/unit/core/functools/test_decorator_location.py 100.00% <100.00%> (ø)
flytekit/extras/tasks/shell.py 78.16% <0.00%> (-7.78%) ⬇️
flytekit/core/map_task.py 78.89% <0.00%> (ø)
flytekit/tools/translator.py 84.95% <0.00%> (ø)
tests/flytekit/unit/extras/tasks/test_shell.py 100.00% <0.00%> (ø)
tests/flytekit/unit/core/test_conditions.py 98.77% <0.00%> (+0.05%) ⬆️
tests/flytekit/unit/core/test_node_creation.py 95.96% <0.00%> (+0.37%) ⬆️
... and 2 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 64ad510...1b0b1e0. 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 6, 2022 20:29
@wild-endeavor wild-endeavor changed the title [wip] Swap out inspect file location Swap out inspect file location May 6, 2022
Signed-off-by: Yee Hing Tong <[email protected]>
Comment on lines 45 to 48
# import importlib
# importlib.import_module("core.flyte_basics.hello_world")
#
# raise Exception("jfkdlsa")
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you remove those?

flytekit/tools/serialize_helpers.py Outdated Show resolved Hide resolved
tests/flytekit/unit/core/functools/decorator_usage.py Outdated Show resolved Hide resolved
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]>
Signed-off-by: Yee Hing Tong <[email protected]>
@wild-endeavor wild-endeavor merged commit c011ef7 into master May 6, 2022
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.

2 participants