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

fix(sdk): update v2 yaml format #6661

Merged
merged 13 commits into from
Oct 7, 2021
Merged

fix(sdk): update v2 yaml format #6661

merged 13 commits into from
Oct 7, 2021

Conversation

ji-yaqi
Copy link
Contributor

@ji-yaqi ji-yaqi commented Oct 1, 2021

Description of your changes:

  1. Update InputValuePlaceholder, OutputPathPlaceholder, IfpresentPlaceholder and etc. to contain input_name and output_name with aliasing to YAML.
  2. Remove schema_version. Incoming schema will be evaluated using V2 ComponentSpec. If it doesn't fit, V1 will be used.
  3. Remove annotations and labels from Spec.
  4. Change sequence of yaml fields.
  5. Change implementation spec
  6. Use bracket for placeholder

@google-oss-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ji-yaqi ji-yaqi marked this pull request as ready for review October 4, 2021 23:41
Copy link
Member

@chensun chensun left a comment

Choose a reason for hiding this comment

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

Looking much better!

try:
from typing import ForwardRef
except ImportError:
from typing import _ForwardRef as ForwardRef
Copy link
Member

Choose a reason for hiding this comment

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

You can use from pydantic.typing import ForwardRef. That will probably solve your Python3.6 issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like it leads to another error. Tried and realize the pure " " works better than ForwardRef(" ")

input_name: str = pydantic.Field(alias='inputValue')

class Config:
allow_population_by_field_name = True
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think you could add the config in the base class BasePlaceholder so that you don't need to repeat it in the sub-classes. If you need this for all classes, consider create a base model: https://pydantic-docs.helpmanual.io/usage/model_config/#change-behaviour-globally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Used the global base class.

@ji-yaqi ji-yaqi force-pushed the aliasing branch 3 times, most recently from b22bc0d to 7128973 Compare October 6, 2021 17:13
v1_component = _components._load_component_spec_from_component_text(
component_yaml)
return cls.from_v1_component_spec(v1_component)
except:
Copy link
Member

Choose a reason for hiding this comment

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

nit: can we except on a specific exception?

try:
from typing import OrderedDict
except:
from typing import MutableMapping as OrderedDict
Copy link
Member

@chensun chensun Oct 6, 2021

Choose a reason for hiding this comment

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

This might not be a good idea. Python caches imports, even outside this function, the import still exists in some system module dict, it could potentially conflicts with collections.OrderedDict if it was imported as a class (though importing class instead of module is discourage).

If there's no easy way to address this, maybe we just use MutableMapping, and leave a comment above saying we should update this once we drop Python 3.6 or something.

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 used Dict for now and add a comment :) Thanks!

@google-oss-robot
Copy link

@ji-yaqi: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
kubeflow-pipelines-samples-v2 d347b62 link true /test kubeflow-pipelines-samples-v2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@chensun
Copy link
Member

chensun commented Oct 7, 2021

/lgtm
/approve

Thanks!

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

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

@chensun chensun merged commit 5892bf9 into kubeflow:master Oct 7, 2021
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