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

Remove cached binaries on worker startup #1667

Merged
merged 2 commits into from
Aug 20, 2023

Conversation

vondele
Copy link
Member

@vondele vondele commented May 5, 2023

as the user could change the compiler at worker startup time (e.g. clang vs gcc), the binary cache might contain binaries compiled with a different compiler than the ones generated by the worker. As different compilers lead to binaries of different speed, test results could be biased, unless we assure that the same compiler is used for both base and new test.

This patch just clears the binary cache once at worker startup, leaving the caching mechanism unchanged otherwise.

Possibly the binary cache should also be flushed on worker update (e.g. the architecture selection logic might change). This is not implemented.

fixes #1666

@vondele
Copy link
Member Author

vondele commented May 5, 2023

If was looking at flushing the cache also after a worker update, but was somewhat confused by https://github.com/glinscott/fishtest/blob/52a42301c2214b5c8004362fcef95ec46f9596c0/worker/worker.py#L1282-L1283

will this always print the failure message? I might not know how exactly the worker update goes.

@vdbergh
Copy link
Contributor

vdbergh commented May 5, 2023

No because a successful update does not return (as written in the comment…).

@vondele
Copy link
Member Author

vondele commented May 5, 2023

OK, so for the purpose of this patch, a worker update can be treated as a fresh start of the worker.

@vdbergh
Copy link
Contributor

vdbergh commented May 5, 2023

What if the user updates his system compiler while the worker is running? Not an unlikely scenario with auto updates…

@vondele
Copy link
Member Author

vondele commented May 5, 2023

not impossible, but usually these are rare and patch level updates, i.e. wrong code bug fixes that have typically no real performance impact. I'm mostly worried about the clang / gcc switch that the user can access.

We could look at the compiler version, but that wouldn't capture the assembler or the linker, or libc, or lpthreads, or anything else that might have an impact on the generated binary.

Copy link
Collaborator

@ppigazzini ppigazzini left a comment

Choose a reason for hiding this comment

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

LG with DEV and windows/linux workers

@ppigazzini
Copy link
Collaborator

Possibly the binary cache should also be flushed on worker update (e.g. the architecture selection logic might change). This is not implemented.

The worker update moves the testing folder to start from scratch with engines, books, nets and cutechess-cli binary. It even deletes the old engine binaries and nets to free up the HD space.

https://github.com/glinscott/fishtest/blob/d00af797fb4cc1b813a81af21b8d119acf799b47/worker/updater.py#L65-L85

as the user could change the compiler at worker startup time (e.g. clang vs gcc),
the binary cache might contain binaries compiled with a different compiler
than the ones generated by the worker. As different compilers lead to binaries of
different speed, test results could be biased, unless we assure that the same compiler
is used for both base and new test.

This patch just clears the binary cache once at worker startup,
leaving the caching mechanism unchanged otherwise.

fixes official-stockfish#1666
@ppigazzini ppigazzini merged commit 528bbaa into official-stockfish:master Aug 20, 2023
17 checks passed
@ppigazzini
Copy link
Collaborator

I triggered the workers update, I will keep on eye on noob's workers, last time they got stuck during the update.
Thank you @vondele :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement worker update code changes requiring a worker update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete binaries cache on worker startup
3 participants