[BUG] Flytekit model file organization improvement

[aeioulisa]

TL;DR

Clean up the structure to align with Flyte IDL.

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

Tracking Issue

flyteorg/flyte#803

[welcome]

Thank you for opening this pull request! 🙌
These tips will help get your PR across the finish line:

• Most of the repos have a PR template; if not, fill it out to the best of your knowledge. - Sign off your commits (Reference: DCO Guide).

[kumare3]

Omg this is a huge change

[wild-endeavor]

Thanks for the PR @aeioulisa! Can you resolve the merge conflict though please? Tests won't run with that there. Thanks.

[eapolinario]

The integration tests under https://github.com/flyteorg/flytekit/tree/master/tests/flytekit/integration/remote build an image that uses an official release of flytekit. The errors in CI are caused by this mismatch, since we're refactoring some files while the remote integration tests are still using the old module names, for example in https://github.com/flyteorg/flytekit/blob/master/tests/flytekit/integration/remote/mock_flyte_repo/workflows/basic/child_workflow.py#L2.

[aeioulisa]

@kumare3 @wild-endeavor @eapolinario Thanks for your review and comments.
I have resolved the conflict.

[wild-endeavor]

Thank you for all this work!!! There's something weird going on with integration tests on master. Let us resolve that first and we'll re-run the tests here.

[wild-endeavor]

hey @aeioulisa can you give @eapolinario and me write permissions to your fork please? want to push a commit to this pr that will make the tests pass. it's a bit of a hack, might need a couple tries to get it right.

thanks!

[eapolinario]

@aeioulisa , can you revert this change? For complicated reasons, in our integration tests we use a pinned version of flytekit, so changing this specific file will need to happen in a separate PR (that either @wild-endeavor or myself are going to handle). This should unblock all tests.

[codecov]

Codecov Report

Merging #706 (74f1433) into master (782bb34) will increase coverage by 0.04%.
The diff coverage is 95.58%.

@@            Coverage Diff             @@
##           master     #706      +/-   ##
==========================================
+ Coverage   85.76%   85.80%   +0.04%     
==========================================
  Files         358      358              
  Lines       29778    29793      +15     
  Branches     2428     2428              
==========================================
+ Hits        25538    25564      +26     
+ Misses       3601     3590      -11     
  Partials      639      639              

Impacted Files	Coverage Δ
flytekit/common/component_nodes.py	82.66% <ø> (ø)
flytekit/common/mixins/launchable.py	70.58% <ø> (ø)
flytekit/engines/common.py	79.60% <ø> (ø)
flytekit/models/admin/matchable_resource.py	75.91% <ø> (ø)
flytekit/models/admin/node_execution.py	89.61% <ø> (ø)
flytekit/models/admin/project.py	100.00% <ø> (ø)
flytekit/models/admin/schedule.py	90.14% <ø> (ø)
flytekit/models/common.py	89.09% <ø> (-7.31%)	⬇️
flytekit/models/core/security.py	86.84% <ø> (ø)
flytekit/models/plugins/array_job.py	91.66% <ø> (ø)
... and 148 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 782bb34...74f1433. Read the comment docs.

[wild-endeavor]

Hey @aeioulisa - just to recap, @eapolinario and I

• reverted one line in the child_workflow.py file (because of the way tests work, it's actually using a released version of flytekit - separate issue for another day). we'll put in a PR that corrects it after this one is merged.
• Temporarily removing papermill from plugin testing. It has a dependency on the flytekit spark, which is again looking up a released flytekit, which doesn't have the necessary import changes.
• Ran make fmt - this runs a python formatting tool
• Updated a couple imports in a couple plugins

I'm okay merging this PR as is, but there is one thing we noticed when going through the PR - a lot of the non-model code now imports the models in two different styles. For instance in component_nodes.py, there is both import flytekit.models.core.task and from flytekit.models.core import workflow as _workflow_model .

In some files the same module is being imported - for example test_common imports admin both as flytekit.models.admin.common and from flytekit.models.admin import common as _admin_common.

Just doing a ripgrep, there seems to be a fair number of files where this is the case, but would you be able to change these back to the from x.y import z style of import?

Thank you all the work aligning the models though! After this is in, we'll most likely move all the models into the flyteidl repo so that they don't drift again.

[welcome]

Congrats on merging your first pull request! 🎉