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.

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 module name parameter for workflows and tasks #745

Closed
wants to merge 8 commits into from

Conversation

YmirKhang
Copy link
Contributor

TL;DR

This PR adds an optional parameter to workflow and task types to make the module name explicitly available.

@task(
    cache_version="1",
    container_image="{{.images.default.fqn}}:{{.images.default.version}}",
    module_name="<app.workflows.workflow_file>",
)
def my_task() -> None:
...

...
@workflow(module_name="<app.workflows.workflow_file>")
def my_workflow() -> None:

So that it can be avoided that the workflows and tasks default to __main__.my_task and __main__.my_workflow, by overriding module name in tasks and workflows

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

Complete description

Added extra optional parameter module_name to workflow and task definitions.

Tracking Issue

flyteorg/flyte#1813

Follow-up issue

NA

Signed-off-by: Emirhan Karagül <emirhan350z@gmail.com>
Signed-off-by: Emirhan Karagül <emirhan350z@gmail.com>
Signed-off-by: Emirhan Karagül <emirhan350z@gmail.com>
Signed-off-by: Emirhan Karagül <emirhan350z@gmail.com>
@YmirKhang
Copy link
Contributor Author

This is just an implementation to make it work with a self contained file. This was not tested, also other side effects are not considered. But since the parameter is optional it should be backward compatible.

Signed-off-by: Emirhan Karagül <emirhan350z@gmail.com>
Signed-off-by: Emirhan Karagül <emirhan350z@gmail.com>
Signed-off-by: Emirhan Karagül <emirhan350z@gmail.com>
@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #745 (97b0e29) into master (e6b8610) will increase coverage by 0.03%.
The diff coverage is 92.30%.

❗ Current head 97b0e29 differs from pull request most recent head 10bc3c7. Consider uploading reports for the commit 10bc3c7 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #745      +/-   ##
==========================================
+ Coverage   85.60%   85.63%   +0.03%     
==========================================
  Files         342      342              
  Lines       28926    28989      +63     
  Branches     2381     2378       -3     
==========================================
+ Hits        24763    24826      +63     
- Misses       3530     3531       +1     
+ Partials      633      632       -1     
Impacted Files Coverage Δ
flytekit/core/task.py 80.48% <ø> (ø)
flytekit/core/python_function_task.py 84.25% <75.00%> (-0.36%) ⬇️
flytekit/core/workflow.py 89.18% <80.00%> (-0.18%) ⬇️
...ts/flytekit/unit/core/test_python_function_task.py 92.00% <83.33%> (-1.66%) ⬇️
tests/flytekit/unit/remote/test_remote.py 99.02% <96.29%> (-0.98%) ⬇️
flytekit/core/tracker.py 82.08% <100.00%> (-0.99%) ⬇️
flytekit/types/file/file.py 90.44% <0.00%> (-0.41%) ⬇️
tests/flytekit/unit/core/test_type_engine.py 99.71% <0.00%> (ø)
flytekit/remote/remote.py 70.81% <0.00%> (+0.67%) ⬆️
... and 1 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 e6b8610...10bc3c7. Read the comment docs.

Signed-off-by: Emirhan Karagül <emirhan350z@gmail.com>
@YmirKhang YmirKhang closed this Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant