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 default project domain in execute launch plan #1894

Merged
merged 3 commits into from
Oct 20, 2023

Conversation

troychiu
Copy link
Member

@troychiu troychiu commented Oct 15, 2023

TL;DR

  1. add default project domain in execute launch plan
  2. add name as a parameter

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

When I executed a launch plan with FlyteRemote, it didn't run with default project domain of the remote but needed specified project domain. So, I fixed it with if there is no specified project domain, use default project domain of the remote.

Tracking Issue

flyteorg/flyte#4249

Follow-up issue

NA

Copy link
Contributor

@wild-endeavor wild-endeavor left a comment

Choose a reason for hiding this comment

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

should we just add this to the top level def execute instead?

@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (54e68e0) 55.23% compared to head (289f6ae) 55.23%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1894      +/-   ##
==========================================
- Coverage   55.23%   55.23%   -0.01%     
==========================================
  Files         300      300              
  Lines       22381    22385       +4     
  Branches     3359     3359              
==========================================
+ Hits        12363    12364       +1     
- Misses       9856     9859       +3     
  Partials      162      162              
Files Coverage Δ
flytekit/remote/remote.py 20.95% <0.00%> (-0.13%) ⬇️

... and 1 file with indirect coverage changes

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

@troychiu
Copy link
Member Author

should we just add this to the top level def execute instead?

Hi @wild-endeavor, do you mean we can resolve the identifiers before actually classify different types of entities?
For example, move the resolved_identifiers = self._resolve_identifier_kwargs(entity, project, domain, name, version)
to

flytekit/remote/remote.py Show resolved Hide resolved
flytekit/remote/remote.py Outdated Show resolved Hide resolved
Signed-off-by: troychiu <[email protected]>
Signed-off-by: troychiu <[email protected]>
@troychiu troychiu marked this pull request as ready for review October 17, 2023 08:25
@pingsutw pingsutw merged commit 68de3b3 into flyteorg:master Oct 20, 2023
68 of 70 checks passed
troychiu added a commit to troychiu/flytekit that referenced this pull request Oct 20, 2023
troychiu added a commit to troychiu/flytekit that referenced this pull request Oct 20, 2023
Future-Outlier pushed a commit to Future-Outlier/flytekit that referenced this pull request Oct 20, 2023
eapolinario added a commit that referenced this pull request Nov 1, 2023
* WIP

Signed-off-by: troychiu <[email protected]>

* WIP

Signed-off-by: troychiu <[email protected]>

* fix a test

Signed-off-by: troychiu <[email protected]>

* Fix Makefile and workflow

Signed-off-by: troychiu <[email protected]>

* lint

Signed-off-by: troychiu <[email protected]>

* Update .github/workflows/pythonbuild.yml

Co-authored-by: Kevin Su <[email protected]>
Signed-off-by: troychiu <[email protected]>

* fix bug

Signed-off-by: troychiu <[email protected]>

* fix bug

Signed-off-by: troychiu <[email protected]>

* fix a test

Signed-off-by: troychiu <[email protected]>

* add default project domain in execute launch plan (#1894)

Signed-off-by: troychiu <[email protected]>

* WIP

Signed-off-by: troychiu <[email protected]>

* fix tests

Signed-off-by: troychiu <[email protected]>

* fix test

Signed-off-by: troychiu <[email protected]>

* Use config file in remote

Signed-off-by: Eduardo Apolinario <[email protected]>

* Replace other uses of Config.for_sandbox

Signed-off-by: Eduardo Apolinario <[email protected]>

* Skip joblib integration test

Signed-off-by: Eduardo Apolinario <[email protected]>

* Skip sqlite test

Signed-off-by: Eduardo Apolinario <[email protected]>

* remove legacy

Signed-off-by: troychiu <[email protected]>

---------

Signed-off-by: troychiu <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
ringohoffman pushed a commit to ringohoffman/flytekit that referenced this pull request Nov 24, 2023
ringohoffman pushed a commit to ringohoffman/flytekit that referenced this pull request Nov 24, 2023
* WIP

Signed-off-by: troychiu <[email protected]>

* WIP

Signed-off-by: troychiu <[email protected]>

* fix a test

Signed-off-by: troychiu <[email protected]>

* Fix Makefile and workflow

Signed-off-by: troychiu <[email protected]>

* lint

Signed-off-by: troychiu <[email protected]>

* Update .github/workflows/pythonbuild.yml

Co-authored-by: Kevin Su <[email protected]>
Signed-off-by: troychiu <[email protected]>

* fix bug

Signed-off-by: troychiu <[email protected]>

* fix bug

Signed-off-by: troychiu <[email protected]>

* fix a test

Signed-off-by: troychiu <[email protected]>

* add default project domain in execute launch plan (flyteorg#1894)

Signed-off-by: troychiu <[email protected]>

* WIP

Signed-off-by: troychiu <[email protected]>

* fix tests

Signed-off-by: troychiu <[email protected]>

* fix test

Signed-off-by: troychiu <[email protected]>

* Use config file in remote

Signed-off-by: Eduardo Apolinario <[email protected]>

* Replace other uses of Config.for_sandbox

Signed-off-by: Eduardo Apolinario <[email protected]>

* Skip joblib integration test

Signed-off-by: Eduardo Apolinario <[email protected]>

* Skip sqlite test

Signed-off-by: Eduardo Apolinario <[email protected]>

* remove legacy

Signed-off-by: troychiu <[email protected]>

---------

Signed-off-by: troychiu <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
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.

3 participants