Integrate argument inference #788
Merged
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
RobbeSneyders
requested review from
mrchtr,
GeorgesLorre and
PhilippeMoussalli
RobbeSneyders
commented
Jan 17, 2024
| @@ -20,7 +20,7 @@ def __post_init__(self): | |||
| self.base_image = "fondant:latest" | |||
|
|
|||
|
|
|||
| class PythonComponent: | |||
| class PythonComponent(BaseComponent): | |||
There was a problem hiding this comment.
Needed this to satisfy typing.
| ) | ||
|
|
||
| DockerCompiler().compile(pipeline) |
There was a problem hiding this comment.
This expands the coverage of this test by including the compilation without errors of lightweight components.
| @@ -42,12 +42,12 @@ def wrapper(cls): | |||
|
|
|||
| # updated=() is needed to prevent an attempt to update the class's __dict__ | |||
| @wraps(cls, updated=()) | |||
| class AppliedPythonComponent(cls, PythonComponent): | |||
| class PythonComponentOp(cls, PythonComponent): | |||
There was a problem hiding this comment.
The component is not applied yet at this point.
RobbeSneyders
force-pushed
the
feature/integrate-arg-inference
branch
from
97cc1ea
to
e52a5c4
Compare
PhilippeMoussalli
approved these changes
Jan 18, 2024
examples/sample_pipeline/pipeline.py
Outdated
| return dataframe | ||
|
|
||
|
|
||
| _ = dataset.apply( | ||
| ref=CalculateChunkLength, | ||
| consumes={"text": pa.string()}, | ||
| produces={"chunk_length": pa.int32()}, | ||
| arguments={"feature_name": "chunk_length"}, |
There was a problem hiding this comment.
This is a bit confusing, can't we just keep it as it was before and pass a random string argument
src/fondant/pipeline/pipeline.py
Outdated
| @@ -222,6 +223,7 @@ def from_ref(cls, ref: t.Any, **kwargs) -> "ComponentOp": | |||
| description=description, | |||
| consumes={"additionalProperties": True}, | |||
| produces={"additionalProperties": True}, | |||
| args={k: v.to_spec() for k, v in infer_arguments(ref).items()}, | |||
There was a problem hiding this comment.
Suggested change
| args={k: v.to_spec() for k, v in infer_arguments(ref).items()}, | |
| args={arg_name: arg.to_spec() for arg_name, arg in infer_arguments(ref).items()}, |
just for clarity
mrchtr
approved these changes
Jan 18, 2024
examples/sample_pipeline/pipeline.py
Outdated
| return dataframe | ||
|
|
||
|
|
||
| _ = dataset.apply( | ||
| ref=CalculateChunkLength, | ||
| consumes={"text": pa.string()}, | ||
| produces={"chunk_length": pa.int32()}, | ||
| arguments={"feature_name": "chunk_length"}, |
There was a problem hiding this comment.
nitpick: looks on a first view a bit confusing. Could we pass an dummy argument which is not used and keep dataframe["chunk_length"]?
There was a problem hiding this comment.
great minds think alike
GeorgesLorre
approved these changes
Jan 18, 2024
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.
Follow-up to use the argument inference functionality added in #763