-
Notifications
You must be signed in to change notification settings - Fork 349
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
Conversation
Thanks for making a pull request to Elyra! To try out this branch on binder, follow this link: |
There was a problem hiding this 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
{ | ||
"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" | ||
} |
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
"deprecated": true, |
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)
b4ed8bb
to
94520ad
Compare
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.
94520ad
to
a2430f1
Compare
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. |
Yes, I believe that's the case. Please take a look at elyra-ai/examples#79 to confirm.
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. |
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:
|
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 |
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)
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). |
@ptitzler - close when your PR goes in |
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