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

test: remove cachy dependency and cache compatiblity tests #7437

Merged
merged 5 commits into from
May 23, 2023

Conversation

martin-kokos
Copy link
Contributor

Pull Request Check List

Resolves: kind of related to #5720

Followup of #5868 by chadac which I am happy for. This PR removes cachy dependency and compatibility tests that were put in place for the transition from old cachy.CacheManager to the new utils.cache.FileCache and . Cachy is an abandoned package.

  • Added tests for changed code.
  • Updated documentation for changed code.

@martin-kokos martin-kokos changed the title test: remove cachy dependenyc and cache compatiblity tests test: remove cachy dependency and cache compatiblity tests Jan 30, 2023
@neersighted
Copy link
Member

neersighted commented Jan 30, 2023

I'm not sure this is something we want; at least not for now. cachy is only used at test-time, and I'd rather have the backwards compat tests for a few more releases to ensure we don't break something. We have cache versioning above the on-disk format -- the reason for this code is to make sure we don't choke on the on-disk representation which could be spectacular given that we don't have defensive logic protecting everywhere we use the cache.

To remove cachy, I think I'd prefer to see tests that ensure we gracefully handle a 'corrupt' cache.

@martin-kokos
Copy link
Contributor Author

I'm not sure this is something we want; at least not for now. cachy is only used at test-time, and I'd rather have the backwards compat tests for a few more releases to ensure we don't break something. We have cache versioning above the on-disk format -- the reason for this code is to make sure we don't choke on the on-disk representation which could be spectacular given that we don't have defensive logic protecting everywhere we use the cache.

To remove cachy, I think I'd prefer to see tests that ensure we gracefully handle a 'corrupt' cache.

Alright. I didn't realize how long term the cache actually is. So my understanding now is that a cache generated by older poetry version can be used by current poetry version. I will attempt to look into that graceful handling.

In the meanwhile, would it be acceptable to modify the tests such that cachy test would be skipable? At this time cachy is imported in conftest, so I don't think there is any good way to achieve that.

@Secrus
Copy link
Member

Secrus commented Jan 31, 2023

Alright. I didn't realize how long term the cache actually is. So my understanding now is that a cache generated by older poetry version can be used by current poetry version. I will attempt to look into that graceful handling.

In the meanwhile, would it be acceptable to modify the tests such that cachy test would be skipable? At this time cachy is imported in conftest, so I don't think there is any good way to achieve that.

Would you care to explain why is this such a great matter to you?

@neersighted
Copy link
Member

It's not that the cache is long-lived, is that historically a invalid cache file jams Poetry until the cache is cleared. As such we need to make sure to feed our replacement for Cachy garbage input and make sure that it gracefully handles corrupt files.

@martin-kokos
Copy link
Contributor Author

Alright. I didn't realize how long term the cache actually is. So my understanding now is that a cache generated by older poetry version can be used by current poetry version. I will attempt to look into that graceful handling.
In the meanwhile, would it be acceptable to modify the tests such that cachy test would be skipable? At this time cachy is imported in conftest, so I don't think there is any good way to achieve that.

Would you care to explain why is this such a great matter to you?

I'd like to include poetry in the Gentoo Linux package repo gentoo/gentoo#28843
And because it is desirable to have packages pass their tests in the environment they are installed in, test dependencies need to be satisfied. That would include cachy, but it being an abandoned package and not very popular, I'd like to avoid pulling it into the package repo. It would be acceptable to skip the test involving cachy, but as it is now, I don't think it is possible in a nice way it being imported in conftest. A last resort would be file patching at install time, but that's best to be avoided, and I'd much rather invest the effort in poetry towards as #5720 says rock solid dependencies.

@neersighted
Copy link
Member

Unfortunately, I don't think making our test suite less robust to mitigate distro packaging choices is tenable here in the upstream. If you need to bypass that test, patching it out is your best bet. Or if you want to do something mergable, I'd suggest a robust set of tests with a corpus of good and bad inputs (and maybe even some fuzzing) to ensure that we no longer hang on an invalid cache would be good.

The cachy tests don't prevent the invalid cache issue today, but they do help us avoid making it worse. If we are resilient to an outdated/corrupt on-disk format, that will give us the confidence to drop this set of tests from the test suite.

@martin-kokos
Copy link
Contributor Author

martin-kokos commented Feb 1, 2023

I wouldn't mean to make the tests lest robust in favor of distro packaging. That's why I said, Id rather put the effort into poetry, however unless I am missing something, the new utils.cache.FileCache is already being used now #5868 and the tests that this PR removes merely a cleanup of tests for the transition, proving the interfaces of the two cache implementations is the same.

I can understand the need for a more robust cache, but that concern would belong to #5868 (never mind the fact cachy wasn't in the first place, as you've mentioned) or filed as a separate issue.

I've created #7453 in an attempt to make Cache more resilient to cache file corruption.

@martin-kokos martin-kokos force-pushed the mk/rm_cache_tests branch 2 times, most recently from 4cf4d0d to 733fcf4 Compare March 31, 2023 12:40
@martin-kokos
Copy link
Contributor Author

martin-kokos commented Mar 31, 2023

Rebased and resolved conflicts.

Is there any update on the opinion of not wanting to remove cachy tests (verifying the parity with utils.cache.FileCache) just yet?
If not, would it be acceptable to make cachy on optional dependency achieved with something like pytest.importskip

@martin-kokos martin-kokos force-pushed the mk/rm_cache_tests branch 2 times, most recently from 9977042 to bbdeed6 Compare March 31, 2023 17:21
poetry.lock Outdated Show resolved Hide resolved
@martin-kokos martin-kokos force-pushed the mk/rm_cache_tests branch 3 times, most recently from 0f048d6 to 1059496 Compare April 24, 2023 08:24
@martin-kokos
Copy link
Contributor Author

Rebased and resolved conflicts.

@Secrus
Copy link
Member

Secrus commented May 21, 2023

@martin-kokos could you rebase and resolve conflicts? I guess it's time to merge this

@martin-kokos
Copy link
Contributor Author

🙌 Rebased. Thanks for the ping.
If you could, please, check a newly added commit resolving some typing checks.

@Secrus Secrus enabled auto-merge (squash) May 23, 2023 07:42
@Secrus Secrus merged commit 776a917 into python-poetry:master May 23, 2023
@martin-kokos martin-kokos deleted the mk/rm_cache_tests branch May 26, 2023 11:59
Copy link

github-actions bot commented Mar 3, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants