[components] Update component naming scheme #27512
Open
+231
−174
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This stack of pull requests is managed by Graphite. Learn more about stacking. |
OwenKephart
force-pushed
the
02-03-_components_update_component_naming_scheme
branch
3 times, most recently
from
7d57bfa to
6239e40
Compare
OwenKephart
force-pushed
the
02-03-_components_update_component_naming_scheme
branch
from
6239e40 to
df3cf5a
Compare
schrockn
reviewed
Feb 3, 2025
python_modules/libraries/dagster-components/dagster_components/core/component_decl_builder.py
Outdated
Show resolved
Hide resolved
schrockn
previously requested changes
Feb 3, 2025
|
I was concerned that names like |
OwenKephart
force-pushed
the
02-03-_components_update_component_naming_scheme
branch
3 times, most recently
from
0660c07 to
1f03c36
Compare
OwenKephart
force-pushed
the
02-03-_components_update_component_naming_scheme
branch
from
1f03c36 to
362effc
Compare
schrockn
reviewed
Feb 4, 2025
Comment on lines
+126
to
+128
| def from_string(s: str) -> "ComponentTypeKey": | ||
| name, package = s.split("@") | ||
| return ComponentTypeKey(name=name, package=package) |
There was a problem hiding this comment.
makes sense to have error checking here
schrockn
reviewed
Feb 4, 2025
Comment on lines
+118
to
+120
| class ComponentTypeKey: | ||
| name: str | ||
| package: str |
There was a problem hiding this comment.
I think we need to bikeshed here.
Elsewhere we use the term "component name" to refer to the full string but here name is only the local name. We need to come up with consitent nomenclature.
schrockn
reviewed
Feb 4, 2025
| return f"{self.name}@{self.package}" | ||
|
|
||
| @staticmethod | ||
| def from_string(s: str) -> "ComponentTypeKey": |
There was a problem hiding this comment.
s should have a meaningful name
schrockn
reviewed
Feb 4, 2025
| @@ -324,7 +324,10 @@ def component_check_command( | |||
|
|
|||
| try: | |||
| json_schema = ( | |||
| component_registry.get(component_dir, component_name).component_params_schema or {} | |||
| component_registry.get( | |||
| component_dir, RemoteComponentKey.from_string(component_name) | |||
There was a problem hiding this comment.
example of inconsistent use of component_name
schrockn
reviewed
Feb 4, 2025
Comment on lines
+16
to
+27
| class RemoteComponentKey: | ||
| name: str | ||
| package: str | ||
|
|
||
| @staticmethod | ||
| def from_string(s: str) -> "RemoteComponentKey": | ||
| name, package = s.split("@") | ||
| return RemoteComponentKey(name=name, package=package) | ||
|
|
||
| def to_string(self) -> str: | ||
| return f"{self.name}@{self.package}" | ||
|
|
schrockn
requested changes
Feb 4, 2025
There was a problem hiding this comment.
This is a pretty important PR as it introduces new terms to our ontology and we should do it deliberately.
Things to name
- The full string "foo@bar" (component identifier)?
- The first component of that string (name?)
- The second component of that string (module? code container") It is either a file or a package. Curious what @smackesey thinks
- The data structure that structures "foo@bar"
We should define this ontology, document it, put it in the PR summary, and then use it consistently in the code base.
This was referenced Feb 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary & Motivation
This updates the naming scheme of our components to be
<name>@<namespace>instead of<namespace>.<name>.This also updates some terminology throughout the codepaths to distinguish between the name of a component (which does not include the namespace), and the registry key (which is the unique identifier). Previously, we used the term "name" for both of these cases, which was confusing.
How I Tested These Changes
Changelog
NOCHANGELOG