Skip to content
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

chore: fixing pylint issues #8610

Merged
merged 7 commits into from
Dec 9, 2024
Merged

Conversation

davidsbatista
Copy link
Contributor

@davidsbatista davidsbatista commented Dec 6, 2024

Proposed Changes:

I'm fixing the issue with pylint, which locally runs over all files. Since the last update (I suppose to pylint), it makes the local linting process raise an issue for functions with more than 5 positional arguments, which we have a lot of.

We should probably increase this number to slightly more than 5 in pyproject.toml, e.g.: max-positional-arguments = 10 and then iteratively remove this disablement as we refactor some of the functions.

How did you test it?

I successfully ran pylint on main branch in my local environment without pylint raising any issues.

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

@davidsbatista davidsbatista changed the title initial import chore: fixing pylint issues Dec 6, 2024
@davidsbatista davidsbatista added the ignore-for-release-notes PRs with this flag won't be included in the release notes. label Dec 6, 2024
@davidsbatista davidsbatista marked this pull request as ready for review December 6, 2024 16:14
@davidsbatista davidsbatista requested a review from a team as a code owner December 6, 2024 16:14
@davidsbatista davidsbatista requested review from mpangrazzi and removed request for a team December 6, 2024 16:14
@coveralls
Copy link
Collaborator

coveralls commented Dec 6, 2024

Pull Request Test Coverage Report for Build 12240140835

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 8 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.001%) to 90.335%

Files with Coverage Reduction New Missed Lines %
components/readers/extractive.py 8 95.71%
Totals Coverage Status
Change from base Build 12238133563: 0.001%
Covered Lines: 8038
Relevant Lines: 8898

💛 - Coveralls

@davidsbatista
Copy link
Contributor Author

adding @anakin87 as well

Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this.
Looks good in general, but I left some suggestions.

In the long term, I would prefer to keep the current (default) value of max-positional-arguments, so that when adding new code, we are aware of this and behave correctly.

haystack/components/rankers/meta_field.py Outdated Show resolved Hide resolved
haystack/components/readers/extractive.py Show resolved Hide resolved
haystack/components/readers/extractive.py Outdated Show resolved Hide resolved
haystack/components/readers/extractive.py Outdated Show resolved Hide resolved
@mpangrazzi
Copy link
Contributor

mpangrazzi commented Dec 9, 2024

@davidsbatista @anakin87

We should probably increase this number to slightly more than 5 in pyproject.toml, e.g.: max-positional-arguments = 10 and then iteratively remove this disablement as we refactor some of the functions.

I actually agree on this, maybe we can do it on another PR? WDYT @anakin87? Edit: saw that you don't want to do it.

Other changes LGTM!

@davidsbatista
Copy link
Contributor Author

@mpangrazzi we can do discuss how to do it or do it in another PR.

For now I just wanted to fix this so that I pylint doesn't raise any issues when running it locally.

@davidsbatista
Copy link
Contributor Author

@anakin87 trying to the fix the _postprocess() and _preprocess() from the ExtractiveReader - adding a kwargs * doesn't seem to work as for the others - investigating...

@davidsbatista
Copy link
Contributor Author

************* Module haystack.components.readers.extractive
haystack/components/readers/extractive.py:603:86: E1121: Too many positional arguments for method call (too-many-function-args)
haystack/components/readers/extractive.py:603:86: E1125: Missing mandatory keyword argument 'queries' in method call (missing-kwoa)
haystack/components/readers/extractive.py:603:86: E1125: Missing mandatory keyword argument 'documents' in method call (missing-kwoa)
haystack/components/readers/extractive.py:603:86: E1125: Missing mandatory keyword argument 'max_seq_length' in method call (missing-kwoa)
haystack/components/readers/extractive.py:603:86: E1125: Missing mandatory keyword argument 'query_ids' in method call (missing-kwoa)
haystack/components/readers/extractive.py:603:86: E1125: Missing mandatory keyword argument 'stride' in method call (missing-kwoa)
haystack/components/readers/extractive.py:632:36: E1121: Too many positional arguments for method call (too-many-function-args)
haystack/components/readers/extractive.py:632:36: E1125: Missing mandatory keyword argument 'start' in method call (missing-kwoa)
haystack/components/readers/extractive.py:632:36: E1125: Missing mandatory keyword argument 'end' in method call (missing-kwoa)
haystack/components/readers/extractive.py:632:36: E1125: Missing mandatory keyword argument 'sequence_ids' in method call (missing-kwoa)
haystack/components/readers/extractive.py:632:36: E1125: Missing mandatory keyword argument 'attention_mask' in method call (missing-kwoa)
haystack/components/readers/extractive.py:632:36: E1125: Missing mandatory keyword argument 'answers_per_seq' in method call (missing-kwoa)
haystack/components/readers/extractive.py:632:36: E1125: Missing mandatory keyword argument 'encodings' in method call (missing-kwoa)

@anakin87
Copy link
Member

anakin87 commented Dec 9, 2024

@davidsbatista I pushed a commit (c389a57) to show what I mean: once you modify the method, you should also modify the caller and related tests.

@davidsbatista
Copy link
Contributor Author

ok, I see, thanks 👍🏽

@anakin87 anakin87 self-requested a review December 9, 2024 16:43
Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

🙏

@davidsbatista davidsbatista enabled auto-merge (squash) December 9, 2024 16:44
@davidsbatista davidsbatista merged commit 248dccb into main Dec 9, 2024
18 checks passed
@davidsbatista davidsbatista deleted the fix-too-many-positional-arguments branch December 9, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ignore-for-release-notes PRs with this flag won't be included in the release notes. topic:build/distribution topic:tests type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants