feat: update function signatures to accept **kwargs #2
Conversation
Summary of ChangesHello @Tohrusky, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily enhances the flexibility and maintainability of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates several function signatures to accept **kwargs, which is a good step towards a more flexible API. However, in several places, these **kwargs are not used, which can be misleading. I've left comments on these specific instances.
Additionally, I've noticed a couple of changes that could be improved:
- The
BaseModelInterface.__call__method change introduces boilerplate for subclasses. - The linter configuration for unused imports (
F401) has been disabled globally, which is generally not recommended.
I've provided suggestions on how to address these points. The other changes, like cleaning up imports, look good.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates several function signatures to accept **kwargs, increasing API flexibility. The changes are mostly well-implemented, passing **kwargs down the call stack where appropriate. However, there are a few instances where **kwargs is added to a function signature but remains unused, which can be misleading. I've pointed these out. Additionally, there's a breaking change in BaseModelInterface that removes a useful default implementation for __call__, which I recommend reconsidering. The PR also includes code cleanup, like removing # noqa comments, which is enabled by new linter configurations in pyproject.toml.
|
|
||
| def __call__(self, *args: Any, **kwargs: Any) -> Any: | ||
| return self.inference(*args, **kwargs) | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Changing the implementation of __call__ to raise NotImplementedError from return self.inference(*args, **kwargs) is a significant breaking change for the BaseModelInterface. The previous implementation provided a useful default behavior, making any object that implements inference callable. Now, every subclass must explicitly implement __call__, often with the same boilerplate code that was just removed (return self.inference(*args, **kwargs)), as seen in CCBaseModel. This reduces code reusability and could break other classes that implement this interface and relied on the default __call__ behavior. It's recommended to revert this change to maintain a more robust and developer-friendly interface.
| raise NotImplementedError | |
| return self.inference(*args, **kwargs) |
| def from_pretrained( | ||
| pretrained_model_name: Union[ConfigType, str], | ||
| **kwargs: Any, | ||
| ) -> Any: |
There was a problem hiding this comment.
The **kwargs parameter is added to the function signature but is not used within the function body, which simply returns CONFIG_REGISTRY.get(pretrained_model_name). This can be misleading for users of this method, as it suggests that additional arguments can be passed, but they will be silently ignored. Please consider either using these arguments, removing them if they are not needed, or at least documenting that they are ignored and perhaps logging a warning if they are provided.
| progress: bool = True, | ||
| model_dir: Optional[str] = None, | ||
| gh_proxy: Optional[str] = None, | ||
| **kwargs: Any, |
There was a problem hiding this comment.
The **kwargs parameter is added to the function signature but is not used within the function body. This can be misleading for users of this function. If the intention is to allow for future extensions, this should be documented in the docstring. Otherwise, it should be removed to avoid confusion. Currently, all call sites within the project call this function without any extra keyword arguments.
| pad_img: Optional[Tuple[int, int]] = None, | ||
| model_dir: Optional[str] = None, | ||
| gh_proxy: Optional[str] = None, | ||
| **kwargs: Any, |
There was a problem hiding this comment.
The **kwargs parameter is added to the __init__ method but is not used within this base class. While this is a common pattern to allow subclasses to receive their own specific arguments, it's good practice to update the docstring to clarify that **kwargs are for subclass-specific parameters. This improves clarity for developers using or extending this class.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
1 participant
No description provided.