Update caching arguments to include ComponentOp and the Image class
#802
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
There was a problem hiding this comment.
Updated the parquet file to include more rows since I was running into issue with the small dataset (probably due to empty partitions)
| @@ -94,8 +94,8 @@ def __init__( | |||
| image: str, | |||
| *, | |||
| description: t.Optional[str] = None, | |||
| consumes: t.Optional[t.Dict[str, t.Union[str, pa.DataType]]] = None, | |||
| produces: t.Optional[t.Dict[str, t.Union[str, pa.DataType]]] = None, | |||
| consumes: t.Optional[t.Dict[str, t.Union[str, pa.DataType, bool]]] = None, | |||
There was a problem hiding this comment.
to accommodate for 'additional_field=True`
PhilippeMoussalli
force-pushed
the
update-caching-arguments
branch
from
1ca4479
to
37b1b42
Compare
PhilippeMoussalli
requested review from
RobbeSneyders,
GeorgesLorre and
mrchtr
and removed request for
RobbeSneyders and
GeorgesLorre
GeorgesLorre
approved these changes
Jan 22, 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.
PR that updates the cache key estimation to take into account the
ComponentSpecinstead of theComponentOp(operation op has both the component spec and the innerproducesandconsumesas well as theImageTested on the example pipeline and caching seems to work fine for lightweight component, only issue is that it will always download the requirements (fondant + extra user dependencies) before skipping the execution of the component. This can become frustrating for iterative development with an increase of dependencies and components.
We might be able to mitigate this by providing base images with pre-installed Fondant version and move the installation of the extras after the cache hit estimation. Best to tackle this in a separate PR. Issue ticket here
The PR also include some small changes to function names to best reflect what they do