Skip to content

fix(backend): ignore unknown fields for pb json unmarshaling #11662

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

Merged
merged 1 commit into from
Feb 24, 2025

Conversation

HumairAK
Copy link
Collaborator

@HumairAK HumairAK commented Feb 21, 2025

Description of your changes:

When new proto api fields are added to the spec, these break the driver due to unknown field constraints. For example:

Today the driver understands this kubernetes execution config:

platforms:
  kubernetes:
    deploymentSpec:
      executors:
        exec-some-component:
          configMapAsEnv:
          - configMapName: my-configmap    # this exists in the api today

Now suppose we are on kfp 2.4, and in 2.5 we add support for a new field, deprecated the old field configMapName:

platforms:
  kubernetes:
    deploymentSpec:
      executors:
        exec-some-component:
          configMapAsEnv:
          - configMapName: my-configmap           # this is now deprecated and configNameParameter is preferred
            configNameParameter:
              runtimeValue:
                constant: my-configmap

And we add backend support for the new field in 2.5, and then the functional sdk changes. The pipeline compiled using the new sdk will not work for 2.4 because 2.4 drivers will not be able to unmarshal this field configNameParameter because it does not recognize it. This change will allow older versions to just drop this field, making new api additions more friendly to older kfp versions.

Checklist:

@google-oss-prow google-oss-prow bot requested review from hbelmiro and mprahl February 21, 2025 22:51
@HumairAK HumairAK changed the title fix(backend): ignore unknown feels for pb json unmarshaling fix(backend): ignore unknown fields for pb json unmarshaling Feb 23, 2025
Copy link
Contributor

@hbelmiro hbelmiro left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Collaborator

@mprahl mprahl left a comment

Choose a reason for hiding this comment

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

/lgtm

@HumairAK
Copy link
Collaborator Author

/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HumairAK, mprahl

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

@google-oss-prow google-oss-prow bot merged commit 9afe23e into kubeflow:master Feb 24, 2025
36 checks passed
HumairAK added a commit to HumairAK/data-science-pipelines that referenced this pull request Feb 28, 2025
HumairAK added a commit to HumairAK/data-science-pipelines that referenced this pull request Feb 28, 2025
HumairAK added a commit to HumairAK/data-science-pipelines that referenced this pull request Feb 28, 2025
HumairAK added a commit that referenced this pull request Feb 28, 2025
* chore: Remove License checker (#11609)

* update dependencies

Signed-off-by: Humair Khan <[email protected]>

* remove license checking

Signed-off-by: Humair Khan <[email protected]>

---------

(cherry picked from commit c100648)

Signed-off-by: Humair Khan <[email protected]>

* fix(backend): Replaced hardcoded ServiceAccount with default config (#11578)

(cherry picked from commit 18641e1)

Signed-off-by: Helber Belmiro <[email protected]>
Signed-off-by: Humair Khan <[email protected]>

* fix(backend) fix run retry for argo (#11585)

(cherry picked from commit b131566)

Signed-off-by: arpechenin <[email protected]>
Signed-off-by: Humair Khan <[email protected]>

* fix(backend): parallelFor resolve upstream inputs. Fixes #11520 (#11627)

(cherry picked from commit f7c0616)

Signed-off-by: zazulam <[email protected]>
Signed-off-by: Humair Khan <[email protected]>

* fix(backend): the metacontroller is broken since #11474 (#11608)

* Update cluster-role-binding.yaml

Signed-off-by: Julius von Kohout <[email protected]>

* Create cluster-role.yaml

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

* Update kustomization.yaml

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

* Update stateful-set.yaml

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

---------

(cherry picked from commit a40163f)

Signed-off-by: Julius von Kohout <[email protected]>
Signed-off-by: juliusvonkohout <[email protected]>
Signed-off-by: Humair Khan <[email protected]>

* fix(manifests): Upgrading metacontroller to v4.11.22 (#11656)

(cherry picked from commit ebaaf75)

Signed-off-by: Tarek Abouzeid <[email protected]>
Signed-off-by: Humair Khan <[email protected]>

* fix(backend): ignore unknown fields for pb json unmarshaling (#11662)

(cherry picked from commit 9afe23e)

Signed-off-by: Humair Khan <[email protected]>

* chore: improved securitycontext for mysql (#11678)

* Update mysql-deployment.yaml

Signed-off-by: Julius von Kohout <[email protected]>

* Update mysql-deployment.yaml

Signed-off-by: Julius von Kohout <[email protected]>

* Update mysql-deployment.yaml

Signed-off-by: Julius von Kohout <[email protected]>

---------

(cherry picked from commit 78675b0)

Signed-off-by: Julius von Kohout <[email protected]>
Signed-off-by: Humair Khan <[email protected]>

* chore: partially revert 9afe23e (#11713)

In 9afe23e we introduced blackend dropping of unknown fields for
unmarshalling, but going forward we want to handle this more on a case
by case basis. In the case for driver we should drop them because by
this point the api server has declare the pipeline spec is acceptable,
so the driver should not fail here. As such we keep driver changes, but
revert those utilized by the api server.

(cherry picked from commit 13b8194)

Signed-off-by: Humair Khan <[email protected]>

* fix(backend) fix execution-level retry on the Argo Workflows backend (#11673)

(cherry picked from commit 30210e3)

Signed-off-by: ntny <[email protected]>
Signed-off-by: arpechenin <[email protected]>
Co-authored-by: arpechenin <[email protected]>
Signed-off-by: Humair Khan <[email protected]>

* Limit the number of parallel tests in SDK execution tests (#11680)

Often times, pods could not be scheduled because of insufficient CPU
and the worker would run out of disk space.

Signed-off-by: mprahl <[email protected]>
Signed-off-by: Humair Khan <[email protected]>

* correct kfp deploy images

Signed-off-by: Humair Khan <[email protected]>

---------

Signed-off-by: Humair Khan <[email protected]>
Signed-off-by: Helber Belmiro <[email protected]>
Signed-off-by: arpechenin <[email protected]>
Signed-off-by: zazulam <[email protected]>
Signed-off-by: Julius von Kohout <[email protected]>
Signed-off-by: juliusvonkohout <[email protected]>
Signed-off-by: Tarek Abouzeid <[email protected]>
Signed-off-by: ntny <[email protected]>
Signed-off-by: mprahl <[email protected]>
Co-authored-by: Helber Belmiro <[email protected]>
Co-authored-by: Anton Pechenin <[email protected]>
Co-authored-by: Michael <[email protected]>
Co-authored-by: Julius von Kohout <[email protected]>
Co-authored-by: Tarek Abouzeid <[email protected]>
Co-authored-by: arpechenin <[email protected]>
Co-authored-by: Matt Prahl <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants