Add pythonpath "." before loading modules

[arbaobao]

Tracking issue

Why are the changes needed?

Sometimes User defined Dockfile is missing "ENV PYTHONPATH /root", it will cause execute errors.

What changes were proposed in this pull request?

This change will add pythonpath "." before loading modules

How was this patch tested?

build a docker image with Dockfile missing "ENV PYTHONPATH /root"

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.59%. Comparing base (a8f68d7) to head (4afb6ae).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2673      +/-   ##
==========================================
- Coverage   78.91%   77.59%   -1.33%     
==========================================
  Files         316      278      -38     
  Lines       24965    23394    -1571     
  Branches     4012     4012              
==========================================
- Hits        19702    18152    -1550     
+ Misses       4548     4521      -27     
- Partials      715      721       +6     

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

[pingsutw]

Could you also remove the PYTHONPATH from this dockerfile? Just want to make sure it works in the integration tests after we remove the PYTHONPATH.

[pingsutw]

nit

Suggested change

	sys.path.append(".")
	if "." not in sys.path:
	sys.path.append(".")

[arbaobao]

@pingsutw I has removed the pythonpath in Dockerfile.dev.

[wild-endeavor]

do you know what happens if both the current working directory, as an absolute path, and this new '.' are both in the sys path? does that affect python module loading at all?

[arbaobao]

@wild-endeavor I think append "." into sys.path is not going to affect python module loading. Just to make sure that user defined Dockerfile works without pythonpath=/root

[thomasjpfan]

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)

[wild-endeavor]

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.

[arbaobao]

Got it. Thank you for your advice.

[pingsutw]

Could we add dest_dir to PYTHONPATH as well?

[thomasjpfan]

Could we add dest_dir to PYTHONPATH as well?

Yea, that would give a better experience. In that case, I would inject it into PYTHONPATH in Popen:

flytekit/flytekit/bin/entrypoint.py

Line 545 in 4309c5e

p = subprocess.Popen(cmd)

I'll say do it in another PR, so we can discuss dest_dir independent of this PR.

[arbaobao]

I'll say do it in another PR, so we can discuss dest_dir independent of this PR.

I think i will create another PR to discuss dest_dir.
Some of the test errors are due to this error.

[wild-endeavor]

yeah let's discuss dest dir in another pr... if we ever move to a different model, like one where another worker maybe written in a different language is first in charge of downloading and extracting the package, it may be weird for that worker to be setting the pythonpath.

[arbaobao]

@thomasjpfan @wild-endeavor @pingsutw
I think we can add dest_dir instead of adding "." because if we don't give --destination-dir=/xxx when we executes, it uses the default value "." .

[thomasjpfan]

I think we can add dest_dir instead of adding "." because if we don't give --destination-dir=/xxx when we executes, it uses the default value "." .

I think it's possible to have dest_dir be different from the working directory.

For example, one can set dest_dir=/home/flytekit/my_dest and working directory is /root. There could be python modules in working_dir=/root baked into the image and then fast registration copies files to dest_dir=/home/flytekit/my_dest. In this case, adding /home/flytekit/my_dest to sys.path is not enough. Either the image or flytekit needs to still needs to add /root into the PYTHONPATH.

[arbaobao]

Got it. Thank you. i will create another PR to discuss dest_dir

[eapolinario]

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.

[eapolinario]

fix in #2701.