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

Use local copies of preconfigured runtime components #2272

Closed
wants to merge 3 commits into from

Conversation

lresende
Copy link
Member

@lresende lresende commented Nov 4, 2021

In a gapped environment, the component registry tries to read all remote
components delaying the app initialization which after some time will
cause app failure to load due to timeout.

This PR removes the catalogs that are reading components from github in favor
of local components.

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 Nov 4, 2021

Thanks for making a pull request to Elyra!

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

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.

This is working for me! Just one comment below re: our new schema format. And then it looks like we'll be able to remove the following files as well:

etc/config/metadata/component-registries/elyra-airflow-filesystem-preconfig.json
etc/config/metadata/component-registries/elyra-airflow-url-preconfig.json
etc/config/metadata/component-registries/elyra-kfp-url-preconfig.json

Comment on lines 1 to 12
{
"display_name": "Airflow Preloaded Components - Directory",
"metadata": {
"description": "Preloaded directory-based components that are supported by Apache Airflow",
"runtime": "airflow",
"location_type": "Directory",
"categories": ["Preloaded Airflow"],
"paths": ["airflow"]
},
"schema_name": "component-registry"
}
Copy link
Member

Choose a reason for hiding this comment

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

There are a few changes needed here based on the new schemas introduced by BYO catalog types

{
  "display_name": "Airflow Preloaded Components - Directory",
  "metadata": {
    "description": "Preloaded directory-based components that are supported by Apache Airflow",
    "runtime": "airflow",
    "categories": ["Preloaded Airflow"],
    "paths": ["airflow"]
  },
  "schema_name": "local-directory-catalog",
  "version": 1
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't we still need the location_type?

Also, should component-registry be called component-catalog?

Copy link
Member

@kevin-bates kevin-bates Nov 6, 2021

Choose a reason for hiding this comment

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

Don't we still need the location_type?

That is a function of the catalog connector now. Each connector is aware of its location type.

Also, should component-registry be called component-catalog?

This schema is deprecated as of this PR:

That said, we'd like to rename the schemaspace from component-registries to component-catalogs but you can't rename schemaspaces (today). Instead, we're likely going to introduce a new schemaspace and deprecate component-registries - which it (and the component-registry schema) should stay around for a release to properly handle migration. (see #2282)

@kevin-bates
Copy link
Member

I just realized that I believe this is essentially a duplicate of elyra-ai/examples#79 (or will result in the same behavior - local components).

cc: @ptitzler

@lresende
Copy link
Member Author

lresende commented Nov 7, 2021

I just realized that I believe this is essentially a duplicate of elyra-ai/examples#79 (or will result in the same behavior - local components).

cc: @ptitzler

Yes, that's what we discussed at the last dev meeting? This PR will stop the retrieval of remote components to enable support in a gapped environment and @ptitzler PR will make the more long-term solution and make Elyra dependable on the other pip package with the sample components.

In gapped environment, the component registry tries to read all remote
components delaying the app initialization which after some time will
cause app failure to load due to timeout.
@kevin-bates
Copy link
Member

@ptitzler PR will make the more long-term solution and make Elyra dependable on the other pip package with the sample components.

Correct. The examples will be part of the 3.3 release (at least that's my understanding and we should defer 3.3 to make that the case), so I don't understand why we need the changes in this PR since it's only going to cause GitHub thrashing and, essentially pollute folks' installation areas with more orphaned files for those installing a build prior to the final 3.3.

@lresende
Copy link
Member Author

lresende commented Nov 7, 2021

I thought @ptitzler was not targeting 3.3 but 3.4. If that is not the case please let me know.

By the way, @ptitzler pr should mimic this pr and not try to fetch components from the internet.

@kevin-bates
Copy link
Member

By the way, @ptitzler pr should mimic this pr and not try to fetch components from the internet.

Yes, I believe that's the case. Please take a look at elyra-ai/examples#79 to confirm.

I thought @ptitzler was not targeting 3.3 but 3.4. If that is not the case please let me know.

Given how close the examples PR is to completion based on the demos I've seen (but sans the minor adjustments necessary for runtime-types and introduction of a component-catalogs schemaspace) I would strongly urge us to include these changes in 3.3.

@ptitzler
Copy link
Member

ptitzler commented Nov 8, 2021

Initially I was thinking it could be deferred to 3.4 but I've found more reasons why we should target 3.3. I have an improved version in the works that addresses most concerns that were raised:

  • separate installations for KFP and Airflow (users are likely to use one or the other, not both)
  • almost completely pre-filled metadata definitions
  • documentation on how to customize the examples [for "personal" use]
  • documentation on how to use the connector as a potential backdoor to supporting any source control system as a component catalog

@lresende
Copy link
Member Author

lresende commented Nov 9, 2021

Initially I was thinking it could be deferred to 3.4 but I've found more reasons why we should target 3.3. I have an improved version in the works that addresses most concerns that were raised:

  • separate installations for KFP and Airflow (users are likely to use one or the other, not both)
  • almost completely pre-filled metadata definitions
  • documentation on how to customize the examples [for "personal" use]
  • documentation on how to use the connector as a potential backdoor to supporting any source control system as a component catalog

Could you clarify what do you mean by your first bullet?

Also want to make sure there is no remote fetching of components on these samples

@ptitzler
Copy link
Member

ptitzler commented Nov 9, 2021

separate installations for KFP and Airflow (users are likely to use one or the other, not both)

Even though we don't support it today, users should be able to install Elyra with support for KFP and AA, Elyra with KFP support only, and Elyra with AA support only in the future. The original (connector-based) examples packaging only supported the first deployment scenario, which wasn't user friendly. Details are in this elyra-ai/examples#79 (comment)

Also want to make sure there is no remote fetching of components on these samples

This is correct. Please refer to https://github.com/elyra-ai/examples/blob/c63e962d45e7515c3e26d99ce63104163a7b0215/component-catalog-connectors/airflow-example-components-connector/README.md#customize-the-catalog for background information. In air-gapped environment, one would have to make the desired component examples package available locally (via a "local" PyPI or a build from source code).

@akchinSTC
Copy link
Member

@ptitzler - close when your PR goes in

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.

5 participants