-
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
Add pythonpath "." before loading modules #2673
Changes from 7 commits
8e21f18
fe4ac21
865d065
9e7956b
3cb410d
2f0d01d
855d6cc
de9d1df
710d143
8902707
659515c
30dd9ba
c3c4bab
3538338
152d033
4afb6ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -376,6 +376,10 @@ def _execute_task( | |||||||
dynamic_addl_distro, | ||||||||
dynamic_dest_dir, | ||||||||
) as ctx: | ||||||||
import sys | ||||||||
|
||||||||
if "." not in sys.path: | ||||||||
sys.path.append(".") | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's best to be explicit here and add the fully path for the current working directory:
Suggested change
Although, I do not think there is harm to include the working directory twice. If you want to make sure to not add the working directory twice: working_dir = os.getcwd()
if all(os.path.realpath(path) != working_dir for path in sys.path):
sys.path.append(working_dir) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes agreed. This seems safer than adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. Thank you for your advice. |
||||||||
resolver_obj = load_object_from_module(resolver) | ||||||||
# Use the resolver to load the actual task object | ||||||||
_task_def = resolver_obj.load_task(loader_args=resolver_args) | ||||||||
|
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.
removing this broke flyteremote integration tests due to the way tasks+workflows are registered. The tests end up present in the image under the
tests
module, which lives under theflytekit
directory and since we had/flytekit
as part ofPYTHONPATH
the task was able to be run oncepyflyte-execute
ran.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.
fix in #2701.