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

Add docs, unit tests, and refactor repro test infra #3439

Merged
merged 1 commit into from
Nov 16, 2024

Conversation

charleskawczynski
Copy link
Member

@charleskawczynski charleskawczynski commented Nov 15, 2024

This PR adds docs, unit tests, and refactors the reproducibility test infrastructure. In adding unit tests and documentation, I've realized that some of the terminology around the repro tests needs revised for certain terminology to be well defined.

In particular, if we consider the situation where commits are reverted, we can end up with multiple and diverging paths (multiple paths with the same reference counter, but made at different periods in development). For example, if our reference counter history looks like (top being most recent):

137
     /PR10 # is PR6 even valid to compare against, here?
136
     /PR9
     /PR10 # should this compare with PRs 4 and 5?
135  # Reverted
     /PR8 # should this compare with commits in PRs 1,2,3?
138
     /PR7
137
     /PR6
136
     /PR4
     /PR5
135
     /PR1
     /PR2
     /PR3

We could clean up older commits, but I think a safer option is to change the latest_comparable_paths to use the (newly) added function compute_bins. I originally used the term bundle (e.g., compute_bundles), which I'm almost leaning towards returning to. It's a longer name but sounds more...tidy.

This was previously not an issue when latest_comparable_paths was latest_comparable_path, because we always grabbed the most recent matched reference to compare against, eliminating this potential issue.

I'll update the README once we land on a design that is coherent, but I think it might take a bit more refactoring to get there.

Copy link
Member

@Sbozzolo Sbozzolo left a comment

Choose a reason for hiding this comment

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

Thanks! I think this is a big improvement in transparency on what the reproducibility tests are doing.

I just left one comment for a function whole logic I have hard time following

reproducibility_tests/reproducibility_utils.jl Outdated Show resolved Hide resolved
reproducibility_tests/reproducibility_utils.jl Outdated Show resolved Hide resolved
reproducibility_tests/reproducibility_utils.jl Outdated Show resolved Hide resolved
reproducibility_tests/reproducibility_utils.jl Outdated Show resolved Hide resolved
reproducibility_tests/reproducibility_utils.jl Outdated Show resolved Hide resolved
@charleskawczynski charleskawczynski force-pushed the ck/repro_unit_tests branch 4 times, most recently from 37077f2 to efdc3ab Compare November 15, 2024 18:10
Update reproducibility_tests/reproducibility_utils.jl

Co-authored-by: Gabriele Bozzola <[email protected]>

Update reproducibility_tests/reproducibility_utils.jl

Co-authored-by: Gabriele Bozzola <[email protected]>

Update reproducibility_tests/reproducibility_utils.jl

Co-authored-by: Gabriele Bozzola <[email protected]>

Update compute_bins
@charleskawczynski charleskawczynski added this pull request to the merge queue Nov 16, 2024
Merged via the queue into main with commit 8800cf8 Nov 16, 2024
16 checks passed
@charleskawczynski charleskawczynski deleted the ck/repro_unit_tests branch November 16, 2024 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation Reproducibility 🎲 Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants