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.

Sign up for GitHub

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

Jump to bottom

Add pythonpath "." before loading modules #2673

Merged
merged 16 commits into from
Aug 20, 2024
Merged

Add pythonpath "." before loading modules #2673

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
8e21f18
add pythonpath . before loading modules
arbaobao Aug 9, 2024
fe4ac21
Merge branch 'flyteorg:master' into add_pythonpath
arbaobao Aug 9, 2024
865d065
fix make lint failed
arbaobao Aug 10, 2024
9e7956b
Merge branch 'flyteorg:master' into add_pythonpath
arbaobao Aug 10, 2024
3cb410d
Merge branch 'add_pythonpath' of github.com:arbaobao/flytekit into ad…
arbaobao Aug 10, 2024
2f0d01d
improve code and remove pythonpath in Dockerfile.dev
arbaobao Aug 13, 2024
855d6cc
Merge branch 'master' into add_pythonpath
arbaobao Aug 15, 2024
de9d1df
add pythonpath '.' if '.' isn't in sys.path
arbaobao Aug 16, 2024
710d143
Merge branch 'add_pythonpath' of github.com:arbaobao/flytekit into ad…
arbaobao Aug 16, 2024
8902707
remove test
arbaobao Aug 16, 2024
659515c
change adding working_dir to dynamic_dest_dir
arbaobao Aug 18, 2024
30dd9ba
Merge branch 'flyteorg:master' into add_pythonpath
arbaobao Aug 18, 2024
c3c4bab
rollback changes back to previous
arbaobao Aug 18, 2024
3538338
Merge branch 'add_pythonpath' of github.com:arbaobao/flytekit into ad…
arbaobao Aug 18, 2024
152d033
lint
arbaobao Aug 18, 2024
4afb6ae
add working_dir in map task
arbaobao Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions Dockerfile.dev
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,5 @@ RUN SETUPTOOLS_SCM_PRETEND_VERSION_FOR_FLYTEKIT=$PSEUDO_VERSION \
&& chown flytekit: /home \
&& :


ENV PYTHONPATH="/flytekit:/flytekit/plugins/flytekit-k8s-pod:/flytekit/plugins/flytekit-deck-standard:"
Copy link
Collaborator

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 the flytekit directory and since we had /flytekit as part of PYTHONPATH the task was able to be run once pyflyte-execute ran.

Copy link
Collaborator

Choose a reason for hiding this comment

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

fix in #2701.


# Switch to the 'flytekit' user for better security.
USER flytekit
4 changes: 4 additions & 0 deletions flytekit/bin/entrypoint.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(".")
Copy link
Member

Choose a reason for hiding this comment

The 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
if "." not in sys.path:
sys.path.append(".")
sys.path.append(os.getcwd())

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes agreed. This seems safer than adding .. I'm worried about users who use both .relative imports as well as absolute imports. We've seen weird cases of repeat-importing of modules before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
Expand Down
Loading