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

Deprecate KFP v1 SDK support #6924

Merged
merged 15 commits into from
Oct 9, 2024
Merged

Deprecate KFP v1 SDK support #6924

merged 15 commits into from
Oct 9, 2024

Conversation

nikelite
Copy link
Contributor

@nikelite nikelite commented Oct 2, 2024

New TFX version discontinues KFP v1 SDK support.

@keerthanakadiri keerthanakadiri self-assigned this Oct 4, 2024
@keerthanakadiri keerthanakadiri requested a review from briron October 4, 2024 04:11
Copy link
Member

@briron briron left a comment

Choose a reason for hiding this comment

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

Thanks for the work! However, some tests failed or were canceled. Do you have any idea why?

Copy link
Member

Choose a reason for hiding this comment

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

It is probably not related to this PR, but the test tests (3.10, not e2e, DEFAULT) is failing in this PR because of KEYERROR, and _BASE_CONTAINER_IMAGE = os.environ['KFP_E2E_BASE_CONTAINER_IMAGE'] seems to be a culprint of the failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should update some CI settings and/or some test utility files. Since py_test collects non-e2e tests at runtime, the unit tests are complaining that there are no valid environments.

cc: @peytondmurray

Copy link
Contributor

@peytondmurray peytondmurray Oct 4, 2024

Choose a reason for hiding this comment

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

Tagging @smokestacklightnin and @aktech for input

Copy link
Contributor

@peytondmurray peytondmurray Oct 4, 2024

Choose a reason for hiding this comment

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

Hmm, I think these environment variables are for telling the tests what external docker containers are running so that various external services can be called. I don't think we should be collecting these tests, as we are not currently running any docker containers during the testing CI jobs. Let me make a PR...

@peytondmurray
Copy link
Contributor

@nikelite Normally I'd make a PR against your feature branch, but you're merging your master into the upstream master, so I've just made a PR against your master: nikelite#1

If you merge that, the tests should pass here. The exception will be with the NIGHTLY tests; our dependencies are in a weird state where their allowed versions are not precisely enough defined for pip's dependency solver to quickly find a solution, nor are they loosely defined enough for a solution to be easily found. As a result the NIGHTLY tests spend a huge amount of time downloading tons of versions of the various dependencies to try to find a solution. This is something we should make an issue for because currently it blocks folks from using NIGHTLY.

@nikelite nikelite merged commit 79f378e into tensorflow:master Oct 9, 2024
10 of 14 checks passed
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.

4 participants