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

Revert "Add a bound to the inference tips cache (#1565)" #1570

Merged
merged 1 commit into from
May 17, 2022

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented May 16, 2022

This reverts commit 95bbd5e (#1565).

@cdce8p reported a regression with that commit, and after moving the popitem() call out of the except, and also trying without --jobs=n, I saw no improvement. So reverting.

Despite that, I feel like it's a similar class of issue to the several issues in pylint with --jobs: we need a better understanding on how the global states are managed and interact with --init-hook and the rest. E.g. pylint-dev/pylint#4874

@jacobtylerwalls jacobtylerwalls added the Maintenance Discussion or action around maintaining astroid or the dev workflow label May 16, 2022
@jacobtylerwalls jacobtylerwalls added this to the 2.12.0 milestone May 16, 2022
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2335351008

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.003%) to 91.891%

Totals Coverage Status
Change from base Build 2335079953: -0.003%
Covered Lines: 9202
Relevant Lines: 10014

💛 - Coveralls

@DanielNoord
Copy link
Collaborator

we need a better understanding on how the global states are managed and interact with --init-hook and the rest.

It's not really my interest atm, but I think it would be better if we could remove most of those global states. I believe the reason why we do use those states is to 1) avoid some duplicate initialisation and 2) make combination of results easier. But I think we could just as well initialise a new linter for every process without storing it on some global object.

Too bad this didn't work out, it seemed like it should have worked!

@DanielNoord
Copy link
Collaborator

Btw, just one thought: did you try to increase the cache size as well? The error Marc reported seems to deal with some inheritance patterns. I have seen earlier regressions where inheritance patterns which were too large tripped up the cache as the first real result was removed due to the number of inferences needed to get the second real result. I don't think that is happening here, but it is worth a try?

@jacobtylerwalls
Copy link
Member Author

I didn't, on the assumption that it would just mask the underlying issue. We also don't have any actual users asking for this feature ATM.

@cdce8p
Copy link
Member

cdce8p commented May 17, 2022

I didn't, on the assumption that it would just mask the underlying issue. We also don't have any actual users asking for this feature ATM.

I agree. It might work for Home Assistant but other, bigger projects might still experience issues. Better to revert it for now.

@cdce8p cdce8p merged commit 33b6c56 into pylint-dev:main May 17, 2022
@jacobtylerwalls jacobtylerwalls deleted the cache-bound-follow-up branch May 17, 2022 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining astroid or the dev workflow Regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants