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

Prevent error popups during KFP component parse #2561

Merged
merged 12 commits into from
Mar 23, 2022
Merged

Prevent error popups during KFP component parse #2561

merged 12 commits into from
Mar 23, 2022

Conversation

kiersten-stokes
Copy link
Member

@kiersten-stokes kiersten-stokes commented Mar 14, 2022 •
edited
Loading

Fixes #2534

What changes were proposed in this pull request?

The palette template is updated here to render component descriptions as json-safe strings. The properties template was updated to cover this scenario in #2467, but this case was missed.

Functionality was also added that checks against a basic 'component schema' for KFP YAML-based components (e.g. ensures component names are strings, etc.). This prevents errors when accessing these variables later during parsing.

Finally, the call to parse (for all runtimes) is also now wrapped in a try/except block to prevent popups in other parsing error situations that are still unaccounted for.

How was this pull request tested?

A relevant existing test was updated to cover this case (which includes an update to the kfp_test_operator.yaml resource in /elyra/tests/pipeline/resources/components). The Airflow parser currently does not support component descriptions so no test was added for Airflow. Also tested manually.

A new test has also been added for the new component schema validation functionality.

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 this to the 3.7.0 milestone Mar 14, 2022
@elyra-bot
Copy link

elyra-bot bot commented Mar 14, 2022

Thanks for making a pull request to Elyra!

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

@ptitzler
Copy link
Member

I believe the proposed fix is too narrow in scope. The parser still terminates abnormally if the input is not valid YAML, like this XML snippet:

<note>
<from>tester</from>
<to>author</to>
<subject>Can this handle XML?</subject>
<body>Yes it can!</body>
</note>

From the console:

Exception in thread not--valid-yaml-test:
Traceback (most recent call last):
  File "/opt/anaconda3/envs/kiersten-safe/lib/python3.7/threading.py", line 926, in _bootstrap_inner
    self.run()
  File "/opt/anaconda3/envs/kiersten-safe/lib/python3.7/site-packages/elyra/pipeline/component_catalog.py", line 156, in run
    ComponentCache.instance().read_component_catalog(self._catalog)
  File "/opt/anaconda3/envs/kiersten-safe/lib/python3.7/site-packages/elyra/pipeline/component_catalog.py", line 309, in read_component_catalog
    parsed_components = parser.parse(catalog_entry) or []
  File "/opt/anaconda3/envs/kiersten-safe/lib/python3.7/site-packages/elyra/pipeline/kfp/component_parser_kfp.py", line 45, in parse
    if component_yaml.get('description'):
AttributeError: 'str' object has no attribute 'get'

@kiersten-stokes kiersten-stokes changed the title Fix palette template for safe rendering of component descriptions Prevent error popups during KFP component parse Mar 17, 2022
ptitzler
ptitzler reviewed Mar 21, 2022
View reviewed changes
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.

Could you please add one more test for a yaml that is syntactically not correct?

ptitzler
ptitzler approved these changes Mar 22, 2022
View reviewed changes
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!

@akchinSTC akchinSTC merged commit e45c78b into elyra-ai:master Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akchinSTC akchinSTC akchinSTC approved these changes

@ptitzler ptitzler ptitzler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.7.0
Development

Successfully merging this pull request may close these issues.

kfp components with invalid JSON cause popup spam
3 participants
@kiersten-stokes @ptitzler @akchinSTC