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

object inspection: produce deterministic descriptions for nested collection datastructures #11312

Conversation

jayaddison
Copy link
Contributor

Feature or Bugfix

  • Bugfix

Purpose

  • Improve deterministic reproducibility of documentation

Detail

  • util.inspect.object_description already attempts to sort collections, but this can fail
  • This changeset handles the failure case by using string-based object descriptions as a fallback deterministic sort ordering
  • This changeset also adds recursive object-description calls for list and tuple datatypes

Relates

@jayaddison jayaddison changed the title object inspection: produce deterministic descriptions for nested collection datastructures object inspection: produce deterministic descriptions for nested collection types that can be declared as literals Apr 10, 2023
@jayaddison jayaddison changed the title object inspection: produce deterministic descriptions for nested collection types that can be declared as literals object inspection: produce deterministic descriptions for nested collection datastructures Apr 10, 2023
@jayaddison jayaddison force-pushed the issue-11198/deterministic-nested-collection-element-ordering branch from 50c5288 to e0f148e Compare April 10, 2023 16:09
@jayaddison
Copy link
Contributor Author

(force-pushed to edit my commit authorship because I remembered that puzzling over this originated from use of alembic at work)

@jayaddison jayaddison force-pushed the issue-11198/deterministic-nested-collection-element-ordering branch from f1fefd3 to 597703d Compare April 14, 2023 22:22
Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Is this ready to merge?

A

@jayaddison
Copy link
Contributor Author

@AA-Turner I think so, but I'd like to wait until the other changes I have pending for 6.2 are sorted out before merging this one.

(it takes me quite a while to regain context when re-reading about each change, and so I think I can better-support a small number of changes introduced gradually)

@jayaddison
Copy link
Contributor Author

I'll try to be a bit more precise than my previous message: I'd like to wait until after the 6.2.0 release, before this can be merged.

@jayaddison
Copy link
Contributor Author

@AA-Turner do we have any mechanisms to monitor memory usage and/or runtime performance impact of pull requests?

I'm now comfortable with this being merged, but would be more confident if we had some performance statistics available (in particular, it's the collection of seen object identifiers in one of the post-approval commits that I'm wondering about).

@AA-Turner AA-Turner added this to the some future version milestone Apr 29, 2023
@david-a-wheeler
Copy link

I'm really glad to see this work to deterministically reproduce documents. Thank you!

@jayaddison
Copy link
Contributor Author

I'm now comfortable with this being merged, but would be more confident if we had some performance statistics available (in particular, it's the collection of seen object identifiers in one of the post-approval commits that I'm wondering about).

Maybe slightly off-topic, but from looking into pytest-compatible performance testing tools: pytest-austin looks potentially promising to me. I might try it out for some unrelated projects, although would also be glad to help try it out here if that'd be useful.

The pytest-austin repository itself hasn't had much commit/contributor activity, but I don't think that's a negative indicator; it pulls-in a related dependency, austin, to perform the measurement tasks, and that does appear to be actively maintained.

@paravoid
Copy link
Contributor

I'll try to be a bit more precise than my previous message: I'd like to wait until after the 6.2.0 release, before this can be merged.

Should this be merged now then? Thanks!

@jayaddison
Copy link
Contributor Author

This can be merged in my opinion - I've re-reviewed the changes to confirm my sense about that. Whether it should be merged remains up to the maintainers.

sphinx/util/inspect.py Outdated Show resolved Hide resolved
sphinx/util/inspect.py Outdated Show resolved Hide resolved
sphinx/util/inspect.py Outdated Show resolved Hide resolved
sphinx/util/inspect.py Outdated Show resolved Hide resolved
sphinx/util/inspect.py Outdated Show resolved Hide resolved
sphinx/util/inspect.py Outdated Show resolved Hide resolved
tests/test_util_inspect.py Outdated Show resolved Hide resolved
tests/test_util_inspect.py Outdated Show resolved Hide resolved
@david-a-wheeler
Copy link

Thanks so much for considering this! Making descriptions deterministic will make it easier to determine if a given package really was generated by its claimed source. I hope this (or something like it) will be merged.

@jayaddison
Copy link
Contributor Author

Thanks @david-a-wheeler - to be transparent: I can't guarantee that this resolves the only remaining source of non-replicable documentation; there may be other scenarios where generated documentation can differ. Similarly, it may not be the best possible approach to making these descriptions deterministic; but it's an improvement, I think.

@david-a-wheeler
Copy link

@jayaddison - I understand. Improvements are improvements, though!

@AA-Turner AA-Turner merged commit 467e94d into sphinx-doc:master Jul 27, 2023
26 checks passed
@AA-Turner
Copy link
Member

Thanks @jayaddison @lamby @paravoid!

A

@jayaddison jayaddison deleted the issue-11198/deterministic-nested-collection-element-ordering branch July 27, 2023 22:12
@jayaddison
Copy link
Contributor Author

Thank you, @AA-Turner!

@AA-Turner AA-Turner modified the milestones: some future version, 7.2.0 Aug 11, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2023
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.

util.inspect.object_description: does not emit reliable ordering for a set nested within another collection
5 participants