Skip to content

Conversation

@Borda
Copy link
Contributor

@Borda Borda commented Mar 26, 2025

Before submitting
  • Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Fixes # (issue).

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in GitHub issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@Borda Borda requested a review from Copilot March 26, 2025 22:15
Copy link
Contributor

Copilot AI left a 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 fixes and extends the model registry functionality for PyTorch models by refactoring common setup logic into a new _setup method and adding tests to verify push and pull operations.

  • Refactored name and temporary folder handling into a new _setup method in mixins.
  • Added a new DummyTorchModel and test to validate Torch mixin push/pull behavior.
  • Updated test files to import necessary modules and ensure model clean-up.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
tests/integrations/test_cloud.py Added Torch mixin test and DummyTorchModel for registry operations.
src/litmodels/integrations/mixins.py Introduced _setup method and integrated it into push_to_registry.
tests/integrations/test_mixins.py Renamed a test function to better reflect its purpose.

@Borda Borda requested a review from Copilot March 26, 2025 22:17
Copy link
Contributor

Copilot AI left a 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 fixes the Torch mixin integration and adds production tests for both PyTorch and pickle registry mixins. It renames a test function for clearer intent, introduces a shared _setup helper in the mixins, and adds a new test for pushing and pulling a PyTorch model.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
tests/integrations/test_mixins.py Renamed test function to better reflect the push and pull behavior.
tests/integrations/test_cloud.py Added a new test and DummyTorchModel for PyTorch mixin integration.
src/litmodels/integrations/mixins.py Refactored push methods to use a new _setup helper for parsing and validating model names.
Comments suppressed due to low confidence (1)

src/litmodels/integrations/mixins.py:120

  • [nitpick] The registry key is built using the full name while the file name is derived from the extracted model_name; please confirm that this discrepancy is intentional. If consistency is desired, consider using model_name for the registry key as well.
model_registry = f"{name}:{version}" if version else name

@Borda Borda merged commit 36e2a2c into main Mar 26, 2025
42 checks passed
@Borda Borda deleted the fix/mixin-pt branch March 26, 2025 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant