Skip to content

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

carlosrinc
Copy link

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

  • fixes #issue-number

Proposed Changes:

How did you test it?

Notes for the reviewer

Checklist

  • I have read the contributors guidelines and the code of conduct
  • I have updated the related issue with new insights and changes
  • I added unit tests and updated the docstrings
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test: and added ! in case the PR includes breaking changes.
  • I documented my code
  • I ran pre-commit hooks and fixed any 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.
@carlosrinc carlosrinc requested a review from a team as a code owner June 16, 2025 20:52
@carlosrinc carlosrinc requested review from mpangrazzi and removed request for a team June 16, 2025 20:52
@CLAassistant
Copy link

CLAassistant commented Jun 16, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ mpangrazzi
✅ carlosrinc
❌ google-labs-jules[bot]
You have signed the CLA already but the status is still pending? Let us recheck it.

@carlosrinc
Copy link
Author

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.

Copy link
Contributor

@mpangrazzi mpangrazzi left a 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?

@carlosrinc
Copy link
Author

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.
The detailed, component-specific validation logic remains in the private _validate_component_input helper method.
I've also rechecked the tests to ensure they call the public validate_input method.
Let me know if there's anything else.

@mpangrazzi
Copy link
Contributor

@carlosrinc Nice! But it seems you still need to:

@carlosrinc
Copy link
Author

Hi @mpangrazzi, thanks for the feedback!.
Make tests pass (ATM you have a code format issue) ---> Done.

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.

@mpangrazzi
Copy link
Contributor

Make tests pass (ATM you have a code format issue) ---> Done.

There's still a format issue, you probabily need to run hatch run fmt (as stated in our contribution guide).

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.

I think it could be nice to add a release note for a refactoring. An example is here (you can find other ones under releasenotes folder).

@carlosrinc carlosrinc requested a review from a team as a code owner June 20, 2025 13:30
@carlosrinc carlosrinc requested review from dfokina and removed request for a team June 20, 2025 13:30
@carlosrinc
Copy link
Author

Hi @mpangrazzi, thanks for the feedback!. Its done.

@carlosrinc
Copy link
Author

Hi @mpangrazzi, any additional feedbak?. Thanks!.

@mpangrazzi
Copy link
Contributor

@carlosrinc it seems you still have that format issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants