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

Expose API for setting the global "show errors in reprs" flag #1118

Closed
wants to merge 1 commit into from

Conversation

akx
Copy link
Contributor

@akx akx commented Dec 12, 2023

Change Summary

This PR adds a public API to set the global "include documentation URLs in error reprs" flag that was previously only available via the undocumented PYDANTIC_ERRORS_OMIT_URL environment variable (which was only read once as pydantic_core was initialized, and couldn't be changed afterwards).

To do that, the GILOnceCell holding the value was changed to a lower-level GILProtected.

Related issue number

pydantic/pydantic#7485 is tangentially related.

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
    • Docstring added to the .pyi stub.
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @dmontagu

@akx
Copy link
Contributor Author

akx commented Dec 12, 2023

Dear reviewers, please review 😄

Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Merging #1118 (2fb9cc0) into main (7fa450d) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1118      +/-   ##
==========================================
+ Coverage   89.70%   89.75%   +0.04%     
==========================================
  Files         106      106              
  Lines       16364    16371       +7     
  Branches       35       35              
==========================================
+ Hits        14680    14693      +13     
+ Misses       1677     1671       -6     
  Partials        7        7              
Files Coverage Δ
python/pydantic_core/__init__.py 92.59% <ø> (ø)
src/errors/mod.rs 92.30% <ø> (ø)
src/errors/validation_exception.rs 92.36% <100.00%> (+0.83%) ⬆️
src/lib.rs 87.32% <100.00%> (+0.18%) ⬆️

... and 4 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fa450d...2fb9cc0. Read the comment docs.

@akx akx force-pushed the expose-pydantic-errors-omit-url branch from 33c64f1 to b6f8aec Compare December 12, 2023 06:52
Copy link

codspeed-hq bot commented Dec 12, 2023

CodSpeed Performance Report

Merging #1118 will not alter performance

Comparing akx:expose-pydantic-errors-omit-url (2fb9cc0) with main (7fa450d)

Summary

✅ 140 untouched benchmarks

@akx akx force-pushed the expose-pydantic-errors-omit-url branch from b6f8aec to 1f3b2c8 Compare December 12, 2023 07:07
@akx akx force-pushed the expose-pydantic-errors-omit-url branch from 1f3b2c8 to 2fb9cc0 Compare December 12, 2023 07:16
@akx
Copy link
Contributor Author

akx commented Dec 12, 2023

It looks like the Codspeed benchmark is a bit flaky – pushing non-code changes made it oscillate between "perf bad" and "this is fine".

@davidhewitt
Copy link
Contributor

@akx thanks for this proposal. The Codspeed benchmark does indeed have some volatility at the moment, which we hope to improve with #1085

Regarding the feature added here, if I recall correctly @samuelcolvin, @adriangb and myself were discussing something very similar just last week. I think we came to the conclusion that having this as a global setting had some drawbacks.

As an alternative, we wondered if it would make sense to have this as a property on errors, so on each ValidationError you could write e.show_url_in_repr = False (or True) to override the environment variable for that specific error. Would that be suitable for your use case?

@akx
Copy link
Contributor Author

akx commented Dec 12, 2023

I think we came to the conclusion that having this as a global setting had some drawbacks.

It kind of already is a global setting, namely the undocumented environment variable. 😁
I agree global state isn't generally a great idea, though...

[...] on each ValidationError you could write e.show_url_in_repr = False (or True) to override the environment variable for that specific error. Would that be suitable for your use case?

That means I'd have to look through all of my codebase (and dependencies that might use Pydantic), and set the property before anything happens to repr() the error, which sounds pretty tedious.

There's something of a precedent in the form of hide_input_in_errors, but a hide_url_in_errors would also need to be set on each model by hand.

@davidhewitt
Copy link
Contributor

It kind of already is a global setting, namely the undocumented environment variable. 😁

Yes, but an environment variable which we read once rather than mutable state.

That means I'd have to look through all of my codebase (and dependencies that might use Pydantic), and set the property before anything happens to repr() the error, which sounds pretty tedious.

Potentially, what's your use case which means that the environment variable at startup isn't useful to you?

@akx
Copy link
Contributor Author

akx commented Dec 12, 2023

Yes, but an environment variable which we read once rather than mutable state.

If the issue is the possible perf loss from having to read the GILProtected value once per ValidationError.__repr__ call, I'm sure that could be mitigated (e.g. making the value a regular atomic bool).

Potentially, what's your use case which means that the environment variable at startup isn't useful to you?

The environment variable works for me personally, but if the canonical way to disable the URLs app-wide is the envvar, then it should be at least documented 😄

@davidhewitt
Copy link
Contributor

The environment variable works for me personally, but if the canonical way to disable the URLs app-wide is the envvar, then it should be at least documented 😄

I asked @samuelcolvin about this earlier today, agreed we should make this public and documented but would prefer to flip it around to have PYDANTIC_ERRORS_INCLUDE_URL=True/False (to match the argument which is include_url)

@akx akx marked this pull request as draft December 15, 2023 05:49
@akx
Copy link
Contributor Author

akx commented Dec 15, 2023

I asked @samuelcolvin about this earlier today, agreed we should make this public and documented but would prefer to flip it around to have PYDANTIC_ERRORS_INCLUDE_URL=True/False (to match the argument which is include_url)

I can look into making a PR implementing that instead today :)

Emdraftened this for the time being.

akx added a commit to akx/pydantic-core that referenced this pull request Dec 15, 2023
akx added a commit to akx/pydantic-core that referenced this pull request Dec 15, 2023
akx added a commit to akx/pydantic-core that referenced this pull request Dec 15, 2023
akx added a commit to akx/pydantic-core that referenced this pull request Dec 15, 2023
akx added a commit to akx/pydantic-core that referenced this pull request Dec 15, 2023
akx added a commit to akx/pydantic-core that referenced this pull request Dec 15, 2023
akx added a commit to akx/pydantic-core that referenced this pull request Dec 22, 2023
akx added a commit to akx/pydantic-core that referenced this pull request Dec 22, 2023
akx added a commit to akx/pydantic-core that referenced this pull request Dec 22, 2023
akx added a commit to akx/pydantic-core that referenced this pull request Dec 22, 2023
akx added a commit to akx/pydantic-core that referenced this pull request Dec 22, 2023
@davidhewitt
Copy link
Contributor

Superseded by #1123

@davidhewitt davidhewitt closed this Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants