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

Fix PermissionError for inaccessible database path #3924

Merged

Conversation

JonathanPlasse
Copy link
Contributor

@JonathanPlasse JonathanPlasse commented Mar 16, 2024

DirectoryBasedExampleDatabase.__init__() cast path to Path
@JonathanPlasse JonathanPlasse force-pushed the fix-database-permission-error branch 2 times, most recently from a858b3e to 33b1e20 Compare March 16, 2024 14:54
@JonathanPlasse
Copy link
Contributor Author

Should I move the permission database tests to cover to avoid the coverage failure?

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Tests look good, let's move them to cover/ so that we don't need the pragma. Implementation also looks good though I'd put it into the existing function - see below.

hypothesis-python/src/hypothesis/database.py Outdated Show resolved Hide resolved
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Nice work! I've cherry-picked in our weekly dependency update, since you'd already grabbed the new CPython alpha 🙂

@Zac-HD Zac-HD enabled auto-merge March 17, 2024 04:20
@Zac-HD Zac-HD force-pushed the fix-database-permission-error branch from 17d4cc8 to 90fe842 Compare March 17, 2024 05:02
@Zac-HD
Copy link
Member

Zac-HD commented Mar 17, 2024

Looks like I underestimated how tricky the edge cases here would be - I think I've got it now; the idea is that we error out if the user has explicitly asked for a directory-based DB, but do the fallback logic if the type of DB was never specific.

we only ever run the optimizer if there have been target_observations, and so we can save both time and memory if we don't populate the frontier with buffers that are never interesting.
@Zac-HD Zac-HD force-pushed the fix-database-permission-error branch from f3fe28f to c59ba12 Compare March 17, 2024 05:50
@JonathanPlasse
Copy link
Contributor Author

Looks like I underestimated how tricky the edge cases here would be - I think I've got it now; the idea is that we error out if the user has explicitly asked for a directory-based DB, but do the fallback logic if the type of DB was never specific.

Yes, that is the idea. Also, if an explicit path is given instead of not_set with ExampleDatabase it will also fall back to the in-memory database.

@Zac-HD
Copy link
Member

Zac-HD commented Mar 17, 2024

Also, if an explicit path is given instead of not_set with ExampleDatabase it will also fall back to the in-memory database.

I don't want to do that - if the user has given us a path, we should either use it or error. Falling back to an in-memory DB is equivalent to silently deleting the database at the end of their session, which would be a pretty nasty surprise.

@JonathanPlasse
Copy link
Contributor Author

I missed the change when you removed _usage_dir next to the ":memory:" condition and uselessly disable the auto-merge to avoid merging it.

@JonathanPlasse
Copy link
Contributor Author

This PR was interesting. Thank you again for your help.

@Zac-HD
Copy link
Member

Zac-HD commented Mar 17, 2024

My pleasure 🙂

It looks like the remaining CI issues are just flaky tests, so I expect we'll merge on retry.

@Zac-HD Zac-HD enabled auto-merge March 17, 2024 06:44
@JonathanPlasse
Copy link
Contributor Author

What do you usually do when you encounter flaky tests in CI?

@Zac-HD
Copy link
Member

Zac-HD commented Mar 17, 2024

Some combination of "hit the retry button" (since I have maintainer permissions to do that), and opening an issue to make the test not flaky. Generally our tests are solid, but the sheer number of changes happening for #3921 have raised the baseline a bit lately 😢

@Zac-HD Zac-HD merged commit d8aadbb into HypothesisWorks:master Mar 17, 2024
51 checks passed
@JonathanPlasse JonathanPlasse deleted the fix-database-permission-error branch March 17, 2024 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PermissionError when Hypothesis database dir path is not accessible
2 participants