Skip to content

Conversation

@kevinmessiaen
Copy link
Member

Description

  • Add args and kwargs to the public methods of the model class and subclass
  • Add backward compatibility tests

Type of Change

  • 📚 Examples / docs / tutorials / dependencies update
  • 🔧 Bug fix (non-breaking change which fixes an issue)
  • 🥂 Improvement (non-breaking change which improves an existing feature)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 🔐 Security fix

@kevinmessiaen kevinmessiaen marked this pull request as ready for review December 5, 2023 13:45
"3.11": ["3.11"],
}
PYTHON_MAJOR_VERSION = ".".join(platform.python_version_tuple()[:2])
BACKWARD_COMPATIBILITY_MODEL_VERSIONS = {"3.9": "3.10", "3.10": "3.10", "3.11": "3.11"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we ensure the backward compatibility here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is a lookup table (which pickled version to open), I haven't pickled in Python 3.9 so 3.9 opens 3.10 pickle.

The real test is can we still load the model and predict and we will get an error if we break it by modifying the model class

Copy link
Member

@Inokinoki Inokinoki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

My only concern is on the unused args and kwargs, which impacts the code smell.
But I think we have to do it if without a better solution...

@kevinmessiaen
Copy link
Member Author

LGTM!

My only concern is on the unused args and kwargs, which impacts the code smell. But I think we have to do it if without a better solution...

Let me change to def method(..., *_args, **_kwargs) to prevent code smells

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@kevinmessiaen kevinmessiaen merged commit 82188c4 into main Dec 5, 2023
@kevinmessiaen kevinmessiaen deleted the feature/gsk-2292-add-args-kwargs-to-the-public-methods-of-the-model-and-all branch December 5, 2023 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants