-
Notifications
You must be signed in to change notification settings - Fork 26
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
Start from dataset schema for lightweight python component consumes
#789
Changes from 21 commits
7a6b78a
c007244
e52a5c4
161f214
66a9103
2e10af1
e8d763f
6619b3a
d898e4a
2d80a77
cef482a
8c9d154
4c97282
3ab1bae
b59fb8c
de5a3c1
3943c4b
d8e5563
85f0994
5b69298
12c6f37
60dc6f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -205,22 +205,39 @@ def from_component_yaml(cls, path, **kwargs) -> "ComponentOp": | |
) | ||
|
||
@classmethod | ||
def from_ref(cls, ref: t.Any, **kwargs) -> "ComponentOp": | ||
def from_ref( | ||
cls, | ||
ref: t.Any, | ||
fields: t.Optional[t.Mapping[str, Field]] = None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer to keep this fields argument out of here since this is specific to the lightweight Python components. Can we move this to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see a straightforwards way of doing this, unless we somehow pass the fields to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My original implementation did this in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, this might make less sense after the refactoring on main since my first commits. If we can address my comment above, it's fine for me to keep it like this for now. Would be good to add the argument to the docstring though. |
||
**kwargs, | ||
) -> "ComponentOp": | ||
"""Create a ComponentOp from a reference. The reference can | ||
be a reusable component name, a path to a custom component, | ||
or a python component class. | ||
|
||
Args: | ||
ref: The name of a reusable component, or the path to the directory containing | ||
a custom component, or a python component class. | ||
fields: The fields of the dataset available to the component. | ||
**kwargs: The provided user arguments are passed in as keyword arguments | ||
""" | ||
if inspect.isclass(ref) and issubclass(ref, BaseComponent): | ||
if issubclass(ref, LightweightComponent): | ||
name = ref.__name__ | ||
image = ref.image() | ||
description = ref.__doc__ or "lightweight component" | ||
|
||
consumes_spec = ( | ||
ref.get_spec_consumes(fields, kwargs["consumes"]) | ||
if fields | ||
else {"additionalProperties": True} | ||
) | ||
|
||
component_spec = ComponentSpec( | ||
name, | ||
image.base_image, | ||
description=description, | ||
consumes={"additionalProperties": True}, | ||
consumes=consumes_spec, | ||
produces={"additionalProperties": True}, | ||
args={ | ||
name: arg.to_spec() | ||
|
@@ -726,6 +743,7 @@ def apply( | |
""" | ||
operation = ComponentOp.from_ref( | ||
ref, | ||
fields=self.fields, | ||
produces=produces, | ||
consumes=consumes, | ||
arguments=arguments, | ||
|
@@ -773,6 +791,7 @@ def write( | |
""" | ||
operation = ComponentOp.from_ref( | ||
ref, | ||
fields=self.fields, | ||
consumes=consumes, | ||
arguments=arguments, | ||
input_partition_rows=input_partition_rows, | ||
|
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.
You can make this a class property by combining the
classmethod
andproperty
decorators.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.
hmm it doesn't seem correct, should I apply
getters
andsetters
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.
It should be reversed, but seems like it only works for Python 3.9 and 3.10 (docs).
Just making it a class attribute could work as well:
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.
hmm I still don't quite follow what should be done here, aren't the class methods needed for the decorators? what's the need for attributes in this example?