-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Refactor PipelineBase._validate_input() method #9522
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
base: main
Are you sure you want to change the base?
Conversation
I've refactored the `PipelineBase._validate_input()` method to improve clarity and maintainability. This involved breaking down the method into smaller helper functions and enhancing error messages for better specificity. I also added comprehensive unit tests to cover various input validation scenarios, ensuring the refactored method behaves as expected.
|
Hi team, just to clarify the status of the CLA: google-labs-jules[bot] was a support tool that generated the commits under my supervision. I, carlosrinc, am the contributor and have already signed the CLA. Please let me know if you need anything else. |
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.
@carlosrinc In general I like the refactoring, but I noticed that PR introduces also a breaking change: the original public validate_input
method has been removed in favour of a private _validate_input
.
However, validate_input
is still being used in both AsyncPipeline.run
and Pipeline.run
, so this change would break existing usage.
Can you iterate in order to keep the refactoring work while not updating public method names?
Hi @mpangrazzi, Thanks for the feedback! I've updated the PR: The validate_input method in PipelineBase is now public again, restoring the original method signature to prevent breaking changes. |
@carlosrinc Nice! But it seems you still need to:
|
Hi @mpangrazzi, thanks for the feedback!. Could you please add the ignore-for-release-notes tag to this PR to fix the CI check?. I can't find the option to do it myself. This is because it's an internal refactoring that doesn't change the user-visible behavior. |
There's still a format issue, you probabily need to run
I think it could be nice to add a release note for a refactoring. An example is here (you can find other ones under |
Hi @mpangrazzi, thanks for the feedback!. Its done. |
Hi @mpangrazzi, any additional feedbak?. Thanks!. |
@carlosrinc it seems you still have that format issue. |
I've refactored the
PipelineBase._validate_input()
method to improve clarity and maintainability. This involved breaking down the method into smaller helper functions and enhancing error messages for better specificity.I also added comprehensive unit tests to cover various input validation scenarios, ensuring the refactored method behaves as expected.
Related Issues
Proposed Changes:
How did you test it?
Notes for the reviewer
Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
and added!
in case the PR includes breaking changes.