Copy user files that were imported by workflow in pyflyte run

[thomasjpfan]

Tracking issue

Related to flyteorg/flyte#5343

Why are the changes needed?

Currently, pyflyte run requires --copy-all when the workflow imports files from other directories. This is a common gotcha because pyflyte run workflow.py works, but pyflyte run --remote workflow.py does not.

What changes were proposed in this pull request?

With this PR:

• We look at all the modules in sys.modules and copy over all files that are necessary for the workflow to run.
• This way pyflyte run --remote workflow.py will work out of the box most of the time.
• Less copying than --copy-all because it only copies over the required files. If you have a project with tests or docs, it will not be copied.

I think the new coping mechanism does what copy_module_to_destination does, but I did not want to adjust it just in case I break anything. I added a new add_imported_modules_from_source to additionally copy over files.

How was this patch tested?

I added unit tests to make sure the copying works. I also had a project with this structure:

.
└── workflows
    ├── __init__.py
    ├── another_module
    │   └── wow.py
    ├── main.py
    └── util.py

and ran pyflyte run --remote workflows/main.py and everything copies correctly. Simple workflow in the root directory also works:

.
├── main.py
└── util.py

[pingsutw]

If we remove copy_module_to_destination, does it work as well? If so, I think we can remove that.

For more context:
copy_module_to_destination will only copy the module to remote if and only if that module has a flyte task/workflow.

in this case, pyflyte run --remote uploads foo_wf and main

└── workflows
    ├── __init__.py
    ├── another_module
    │   └── foo_wf.py
    ├── main.py  # it import foo_wf

I did this because I didn't know how to do add_imported_modules_from_source at that time.

Now you have added it, I think we should be able to remove copy_module_to_destination

[wild-endeavor]

+1, remove this one line and merge. tested this against dev but feel free to again. the only difference i saw is that with the copy_module... call, the main script itself is already in the destination folder, and because it exists it's skipped when add_imported... sees it. Without it, the main file itself is copied by add_imported...

[pingsutw]

If utils imports another local module, will we copy that to the remote storage as well?

.
└── workflows
    ├── __init__.py
    ├── another_module
    │   └── wow.py
    ├── main.py  # only import util.py
    └── util.py # import util2.py
     └── util2.py     

[thomasjpfan]

Any files that is a part of the import chain from the original workflow will be copied over.

Details: If main.py imports from util.py and util.py imports from util2.py then util2.py is part of sys.modules when main.py is loaded. With this PR, if the module is in sys.modules and in the same working directory as main.py, then it will be uploaded.

[pingsutw]

nice, perfect

[codecov]

Codecov Report

Attention: Patch coverage is 14.81481% with 46 lines in your changes missing coverage. Please review.

Project coverage is 46.23%. Comparing base (d802c7e) to head (cd34d90).
Report is 14 commits behind head on master.

Files	Patch %	Lines
flytekit/tools/script_mode.py	13.46%	45 Missing ⚠️
flytekit/remote/remote.py	50.00%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (d802c7e) and HEAD (cd34d90). Click for more details.

HEAD has 3 uploads less than BASE

Flag	BASE (d802c7e)	HEAD (cd34d90)
	5	2

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2663       +/-   ##
===========================================
- Coverage   92.53%   46.23%   -46.31%     
===========================================
  Files          27      187      +160     
  Lines        1206    19162    +17956     
  Branches        0     2766     +2766     
===========================================
+ Hits         1116     8859     +7743     
- Misses         90     9867     +9777     
- Partials        0      436      +436     

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

[thomasjpfan]

@pingsutw I'm concerned with this change. At this point, is the main workflow imported by flytekit?

[pingsutw]

I think so.

flytekit/flytekit/clis/sdk_in_container/run.py

Line 877 in 72da0d0

entity = load_naive_entity(module, exe_entity, project_root)

If not, could we params.computed_params.module to the list as well?

[thomasjpfan]

I added a new get_all_modules function that adds params.computed_params.module if it is not already imported.

[pingsutw]

sgtm, thank you!