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

Address test warnings #2833

Merged
merged 2 commits into from
Jul 19, 2022
Merged

Conversation

kevin-bates
Copy link
Member

This pull request is a "first pass" at addressing the warnings produced following the server CI tests. Prior to this PR 3791 warnings were produced. This PR takes the warnings down to 474 (87%) and when merged with #2829, the warnings will be around 55 (98%). (Note, these values were produced on my local dev env. In running CI on my fork, and without #2829, I still see 2108 warnings, but 474 on my local system. I suspect the additional warnings are related to recent deprecations in tornado regarding event loop management and I'm running with an older version.)

The primary issue here was in handling class initialization parameters within the Configurable class hierarchy. The PR currently has two commits, each taking a different approach to resolving the issue.

  • The first was to add a positional argument (for the most part root_dir) so that it is not included in the **kwargs that flow to the superclasses. This approach probably consisted of fewer changes than the second because that was closer to what was happening before.
  • The second approach was to use **kwargs but pop parameters like root_dir out, so that it doesn't interfere in the superclasses. This approach was chosen because I found it was used within the traitlets package, lending credence to that approach.

I also refactored the PipelineProcessorResponse class, introducing a RuntimePipelineProcessorResponse class. The KFP and Airflow response objects derive from the latter while the Local response object derives directly from PipelineProcessorResponse since it doesn't need the location information in the response and was just passing empty string placeholders for those values. In addition, this class hierarchy now mirrors that of the PipelineProcessor class hierarchy.

In some cases, you'll see the __init__() method completely removed. This is intentional as it is not required if the traitlet (e.g., root_dir) is the only item needed from the **kwargs and is functionality the traitlets package provides automatically.

I'm going ahead and marking #2782 as resolved by this PR since most all of the remaining errors can be attributed to upstream components (Tornado, KFP, and Papermill) and should probably be addressed with separate issues for each.

Resolves: #2782

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.

@elyra-bot
Copy link

elyra-bot bot commented Jul 13, 2022

Thanks for making a pull request to Elyra!

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

@kevin-bates kevin-bates added component:pipeline-runtime issues related to pipeline runtimes e.g. kubeflow pipelines area:back-end kind:CI Impacts continuous integration (build, test, etc.) labels Jul 13, 2022
@ptitzler ptitzler added this to the 3.11.0 milestone Jul 13, 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! Without those warnings, it's now much easier to identify test issues. Much appreciated!

Copy link
Member

@kiersten-stokes kiersten-stokes left a comment

Choose a reason for hiding this comment

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

In some cases, you'll see the init() method completely removed. This is intentional as it is not required if the traitlet (e.g., root_dir) is the only item needed from the **kwargs and is functionality the traitlets package provides automatically.

Cool! LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:back-end component:pipeline-runtime issues related to pipeline runtimes e.g. kubeflow pipelines kind:CI Impacts continuous integration (build, test, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review/address 'Elyra Tests: Test Server' warnings
3 participants