Refactor PipelineBase._validate_input() method#9522
Refactor PipelineBase._validate_input() method#9522carlosrinc wants to merge 9 commits intodeepset-ai:mainfrom
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. |
mpangrazzi
left a comment
There was a problem hiding this comment.
@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. |
|
This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
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.