-
Notifications
You must be signed in to change notification settings - Fork 6
Update pod hashing to remove non-replication concerns from hash #115
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
Conversation
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.
Pull Request Overview
This PR refactors pod hashing to exclude non-replication concerns from the hash computation, focusing on reproducibility requirements. The changes consolidate execution recommendations into a separate structure and update GPU requirements to better reflect code dependencies.
- Removed source commit URL and individual CPU/memory fields from pod hash
- Consolidated CPU and memory recommendations into
RecommendSpecsstruct - Updated GPU requirements to use CUDA version instead of specific models
- Added comprehensive pipeline execution infrastructure with Docker runner
Reviewed Changes
Copilot reviewed 46 out of 47 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/uniffi/model/pod.rs | Core changes to Pod structure, removing hash-affecting fields and adding RecommendSpecs |
| src/uniffi/store/filestore.rs | Updated to handle separate storage of recommendation specs with timestamps |
| tests/store.rs | Updated tests to use new Pod constructor and RecommendSpecs structure |
| src/core/pipeline_runner.rs | New comprehensive pipeline execution infrastructure |
| tests/fixture/mod.rs | Updated fixture functions to use new Pod API and added pipeline test utilities |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Indeed, we should turn your points into issues to be tracked. Namely:
|
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.
We should most definitely turn this into a module so we can have smaller src code dedicated to each operator, especially as we add operators in the future.
Depends on #114
Resolves PIPE-119