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?

misused temp folder and improve arguments

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 21:08
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 issues with the Pickle mixin and updates argument names and temporary folder usage for model registry operations, along with corresponding test updates.

  • Updated argument names (model_name/model_version → name/version) in registry push/pull methods
  • Changed temporary folder creation from using gettempdir to mkdtemp for isolation
  • Added tests to verify push and pull functionality with the updated mixins

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 DummyModel and test for PickleRegistryMixin push/pull
src/litmodels/integrations/mixins.py Renamed arguments and updated temp folder creation; added name check
tests/integrations/test_mixins.py Updated calls to pull_from_registry with new argument names

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Borda Borda requested a review from Copilot March 26, 2025 21:11
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 addresses issues in model registry mixins by updating argument names and improving temporary folder usage, while also adding tests for the Pickle mixin.

  • Renamed parameters (model_name → name, model_version → version) in push/pull registry methods.
  • Added validation in PickleRegistryMixin to disallow ":" in model names and switched to using mkdtemp() for temporary folders.
  • Updated tests to use the new API and verify correct push/pull functionality for both pickle and PyTorch models.

Reviewed Changes

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

File Description
tests/integrations/test_cloud.py Added test for Pickle mixin push/pull and updated imports to reflect new names
src/litmodels/integrations/mixins.py Updated argument names and added name validation in Pickle/PyTorch mixins
tests/integrations/test_mixins.py Adjusted test calls to use the revised argument names
Comments suppressed due to low confidence (1)

src/litmodels/integrations/mixins.py:52

  • [nitpick] The dual assignment to 'name' and 'model_name' in PickleRegistryMixin.push_to_registry may be confusing since the file name is derived from 'model_name' while the registry identifier is later constructed using 'name'. Consider refactoring for clarity by explicitly separating the file name from the registry name.
if name is None:
    name = model_name = self.__class__.__name__

@Borda Borda merged commit 2ce6344 into main Mar 26, 2025
42 checks passed
@Borda Borda deleted the fix/mixin-pickle branch March 26, 2025 22:07
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