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 handling of cos_object_prefix pipeline property #2972

Merged
merged 5 commits into from
Oct 20, 2022

Conversation

kiersten-stokes
Copy link
Member

@kiersten-stokes kiersten-stokes commented Oct 17, 2022

Prior to this PR, pipelines with a cos_object_prefix defined will fail to run on main and the 3.12 release.

What changes were proposed in this pull request?

This PR does a handful of related things. First, the pipeline_parameters property on the pipeline_definition.py Pipeline object is removed - it is no longer necessary now that cos_object_prefix is stored in the pipeline_defaults stanza of the pipeline JSON. (Prior to the changes in #2780, cos_object_prefix existed as a sibling to the pipeline_defaults stanza). The PIPELINE_META_PROPERTIES constant is also no longer needed and is removed. A handful of helper methods are added to component_parameter.py and, as a result, the convert_elyra_owned_properties methods are simplified.

The key-value pairs in the pipeline_defaults stanza are passed to the pipeline.py Pipeline object and accessed by the runtime processors in the same way as before, but now this property is named pipeline_properties rather than pipeline_parameters (in order to prepare for an implementation of #2936).

Of note here is that no pipeline migration is necessary right now. In order to avoid a migration so soon in 3.13 (3.12 just required migration), a workaround is added to the backend that supports cos_object_prefix both as a child of the pipeline_defaults and as a sibling. See pipeline_definition:191:197.

How was this pull request tested?

  • Airflow - a case was added to an existing test to cover cos_object_prefix functionality; existing case that uses the same sample pipeline was also updated accordingly
  • KFP -added a new test (test_create_yaml_complex_pipeline) to cover cos_object_prefix and other functionality; mimics the create_test_file test in test_airflow_processor.py
  • Manual test with cos_object_prefix defined in the pipeline_defaults stanza of the pipeline json
  • Manual test with cos_object_prefix defined in the app_data stanza of the pipeline json

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@kiersten-stokes kiersten-stokes added the kind:bug Something isn't working label Oct 17, 2022
@kiersten-stokes kiersten-stokes added this to the 3.13.0 milestone Oct 17, 2022
@elyra-bot
Copy link

elyra-bot bot commented Oct 17, 2022

Thanks for making a pull request to Elyra!

To try out this branch on binder, follow this link: Binder

@kiersten-stokes kiersten-stokes marked this pull request as ready for review October 18, 2022 18:21
@ptitzler ptitzler added the component:pipeline-editor pipeline editor label Oct 19, 2022
Copy link
Member

@ptitzler ptitzler 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
Member

@akchinSTC akchinSTC left a comment

Choose a reason for hiding this comment

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

FYI, I think using | to represent the typehint Unions only work for python >=3.10.

anywho not a biggie, LGTM

@kevin-bates
Copy link
Member

FYI, I think using | to represent the typehint Unions only work for python >=3.10.

Older python versions can use that syntax with from __future__ import annotations. That said, I've found that the overrides package (which we're exploring) has heartburn equating, for example, signatures of type | None to Optional[type] (or Union[type, None]) and when either signature is not in the local package, you're forced to choose the approach of the foreign signature (speaking from experience). So I guess my point is that we should be consistent. 😄

anywho not a biggie

Agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:pipeline-editor pipeline editor kind:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants