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.

Sign up for GitHub

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

Jump to bottom

Remove additional sources of nondeterminism from benchmark archive #2389

Merged
merged 1 commit into from
May 22, 2017
Merged

Remove additional sources of nondeterminism from benchmark archive #2389

merged 1 commit into from
May 22, 2017

Conversation

str4d
Copy link
Contributor

@str4d str4d commented May 19, 2017

Closes #2388.

@str4d str4d added A-CI Area: Continuous Integration I-performance Problems and improvements with respect to performance A-testing Area: Tests and testing infrastructure labels May 19, 2017
@nathan-at-least nathan-at-least changed the title Remove additional sources of determinism from benchmark archive Remove additional sources of nondeterminism from benchmark archive May 19, 2017
nathan-at-least
nathan-at-least approved these changes May 19, 2017
View reviewed changes
qa/zcash/create_benchmark_archive.py Outdated
'--owner=0', '--group=0',
'--mtime=2017-05-17T00:00:00Z',
'-c', '-T', '-'], stdin=sort.stdout, stdout=subprocess.PIPE)
archive = subprocess.check_output(['gzip', '-9n'], stdin=tar.stdout)
Copy link
Contributor

Choose a reason for hiding this comment

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

@bitcartel suggested switching to xz for better compression. Shall we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appears to be fine for determinism, so I'll update the PR to use it and reviewers can confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that if we change this, we need to update CI again, because it expects .gz when sending to the worker.

@nathan-at-least nathan-at-least added this to the 1.0.9 milestone May 19, 2017
@bitcartel
Copy link
Contributor

fyi - if benchmark folder already exists

Height: 107134
Transactions: 56
Traceback (most recent call last):
  File "qa/zcash/create_benchmark_archive.py", line 245, in <module>
    create_benchmark_archive('0000000007cdb809e48e51dd0b530e8f5073e0a9e9bd7ae920fe23e874658c74')
  File "qa/zcash/create_benchmark_archive.py", line 140, in create_benchmark_archive
    os.mkdir('benchmark')
OSError: [Errno 17] File exists: 'benchmark'

@nathan-at-least
Copy link
Contributor

@bitcartel I consider that acceptable, since this would only be run rarely by someone setting up this new benchmark. What do you think?

@str4d
Copy link
Contributor Author

str4d commented May 19, 2017

I don't want to remove the benchmark folder automatically in case someone happened to be using it for something. The best we could do is clean up the error message, if we feel it isn't already clear enough.

@bitcartel
Copy link
Contributor

Ok, I think it's fine to leave as is.

ageis
ageis approved these changes May 19, 2017
View reviewed changes
Copy link
Contributor

@ageis ageis left a comment

Choose a reason for hiding this comment

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

ACK

@str4d
Remove additional sources of determinism from benchmark archive
08dc788 
The archive has also been moved from .tar.gz to .tar.xz for a
33% reduction in size.

Closes zcash#2388.
@str4d str4d force-pushed the 2388-bench-archive-determinism branch from aa3adaf to 08dc788 Compare May 20, 2017 00:02
bitcartel
bitcartel approved these changes May 20, 2017
View reviewed changes
Copy link
Contributor

@bitcartel bitcartel left a comment

Choose a reason for hiding this comment

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

ACK

@nathan-at-least
Copy link
Contributor

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented May 22, 2017

📌 Commit 08dc788 has been approved by nathan-at-least

@zkbot
Copy link
Contributor

zkbot commented May 22, 2017

⌛ Testing commit 08dc788 with merge 7ea88c9...

zkbot added a commit that referenced this pull request May 22, 2017
@zkbot
Auto merge of #2389 - str4d:2388-bench-archive-determinism, r=nathan-…
7ea88c9 
…at-least

Remove additional sources of nondeterminism from benchmark archive

Closes #2388.
@zkbot
Copy link
Contributor

zkbot commented May 22, 2017

☀️ Test successful - pr-merge
Approved by: nathan-at-least
Pushing 7ea88c9 to master...

@zkbot zkbot merged commit 08dc788 into zcash:master May 22, 2017
@nathan-at-least nathan-at-least moved this from Work Queue to Complete in Development Infrastructure Jul 3, 2017
@str4d str4d deleted the 2388-bench-archive-determinism branch January 27, 2023 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bitcartel bitcartel bitcartel approved these changes

@ageis ageis ageis approved these changes

@nathan-at-least nathan-at-least nathan-at-least approved these changes

Assignees
No one assigned
Labels
A-CI Area: Continuous Integration A-testing Area: Tests and testing infrastructure I-performance Problems and improvements with respect to performance
Projects
No open projects
Development Infrastructure
  
Complete
Milestone
1.0.9
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@str4d @bitcartel @nathan-at-least @zkbot @ageis