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.

Sign up for GitHub

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

Jump to bottom

Refactor BaseOp to context manager pattern #762

Merged
merged 1 commit into from
Feb 5, 2019
Merged

Refactor BaseOp to context manager pattern #762

merged 1 commit into from
Feb 5, 2019

Conversation

hongye-sun
Copy link
Contributor

@hongye-sun hongye-sun commented Feb 1, 2019 •
edited by jlewi
Loading

Changes:

  • Rename BaseOp to KfpExecutionContext
  • Implement KfpExecutionContext with context manager pattern which is more flexible than inheritance pattern.
  • Add context ID support to generate stable ID across retries.
  • Ignore errors in local runs

This change is Reviewable

@hongye-sun hongye-sun requested review from Ark-kun and vicaire February 1, 2019 00:20
@k8s-ci-robot k8s-ci-robot requested a review from IronPan February 1, 2019 00:20
@hongye-sun
Copy link
Contributor Author

/test kubeflow-pipeline-e2e-test

@hongye-sun hongye-sun requested a review from gaoning777 February 2, 2019 00:55
@hongye-sun
Copy link
Contributor Author

@vicaire This PR is mostly same as the BaseOp PR, but I decided to go with context manager pattern to make it easier for user to write operation in a single function. Could you take a some time to take another look of it? Thanks.

@Ark-kun
Copy link
Contributor

Ark-kun commented Feb 2, 2019

@hongye-sun Are all those 23 commits part of this PR changes? Can you rebase it on top of master so that the real changes are clearer to see?

@hongye-sun
Copy link
Contributor Author

sg. rebased it from master.

@vicaire
Copy link
Contributor

vicaire commented Feb 5, 2019

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vicaire

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hongye-sun
Copy link
Contributor Author

/test kubeflow-pipeline-e2e-test

@k8s-ci-robot k8s-ci-robot merged commit cea1632 into kubeflow:master Feb 5, 2019
Linchin pushed a commit to Linchin/pipelines that referenced this pull request Apr 11, 2023
@jlewi
Fix autodeploy job (kubeflow#619)
786cb08 
* There is a bug in the code to detect completed jobs. For completed
  jobs the condition can be of type "Complete". As a result, we aren't
  properly detecting when jobs have been completed and rerunning them
  if needed.

Related to kubeflow#762
HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024
@Udiknedormin
fix(sdk): "when" in some ParallelFor loops (kubeflow#762)
3e4f4d8 
* fix: "when" in some ParallelFor loops

* refactor: split long line
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ark-kun Ark-kun Awaiting requested review from Ark-kun

@vicaire vicaire Awaiting requested review from vicaire

@IronPan IronPan Awaiting requested review from IronPan

@gaoning777 gaoning777 Awaiting requested review from gaoning777

Assignees

@vicaire vicaire

Labels
approved lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hongye-sun @Ark-kun @vicaire @k8s-ci-robot