-
Notifications
You must be signed in to change notification settings - Fork 131
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
Switch from cutechess-cli to fastchess #2119
base: master
Are you sure you want to change the base?
Conversation
The PR is ready for review from the fishtest side, still in a draft state to allow for a couple of fixes and updates to be made to fast-chess. |
f340f91
to
91babcd
Compare
current pushed version runs correctly for fishtest AFAICT. Still not master fast-chess, but nearly there. I think the most important thing now verifying that fast-chess is building with all the version of the compilers we need, and in all environments. Current fishtest version check is gcc >= 7.3 and clang >= 8.0 (current online workers are gcc >= 9.3 and clang >= 13.0). |
updated to master now. |
updated to current fast-chess, verified to compile with gcc 7.3.0 and clang 8.0.0, the fishtest minimum required versions. |
there was some code somewhere which made high concurrency worker only pickup ltc tasks, arguably we don't need that bit anymore with fc, but can be done later |
OK, will remove that, it is a server change. |
4cdb5f2
to
deaf11b
Compare
Should we also first compile the binary with tests, to make sure that everything works on the host ? |
To be considered... will the tests work if fast-chess is compiled with 'USE_CUTE' and how long does it take? |
it is a separate binary which can only run the tests
the tests are rather quick, ~3s.. compiling them takes considerably more time... i need to check if that can be improved
there's a depedency on doctest, but the repo has source code for that |
testing worked locally with gcc 7.3 and clang 8.0, and is simple to add. Trying in CI. working. Timing added is not so important, this happens just once and the binary is reused. |
Lines 1157 to 1180 in 12981ff
if I recall correctly |
Isn't that more a property of SF? I think parameters are integers in SF, or at least only that is well tested. |
|
They consulted their legal department, which points to the UCI spec
|
No way a legal department takes |
ah, you mean in fishtest CI. For workers, I think it is acceptable. |
Mh fastchess updates calculates this as w+l+d.. now what comes to my mind is that maybe some race could lead to flawed updates but this shouldn't happend because all relevant parts are behind a lock. Can you give more information perhaps i.e. was the assertion wrong by 1 or 2? meaning a pair was missed or something was not updated in pairs.. also are you on the latest version of this pr? I think there were issues in the past not sure if they were ever included here or fixed before |
There is nothing wrong with fastchess. In the case of SPSA the worker computes WLD of a task as the sum of the WLD for the "mini tasks" (corresponding to a choice of parameters). The same should be done for the total number of games. |
One may wonder however what the point is of this summing of WLD data corresponding to different sets of parameters... |
oh! i missed the fact that both assertion errors came from a spsa test, thanks |
@vdbergh is this the proper fix (quick local test suggest it is): diff --git a/worker/games.py b/worker/games.py
index 88a26c2..cc82c02 100644
--- a/worker/games.py
+++ b/worker/games.py
@@ -990,7 +990,12 @@ def parse_fastchess_output(
spsa["losses"] = fastchess_WLD_results["losses"]
spsa["draws"] = fastchess_WLD_results["draws"]
- num_games_finished = fastchess_WLD_results["games"]
+ num_games_finished = (
+ fastchess_WLD_results["games"]
+ + saved_stats["wins"]
+ + saved_stats["losses"]
+ + saved_stats["draws"]
+ )
fastchess_ptnml_results = None
fastchess_WLD_results = None |
Yes I think so. |
For reference: there are some more assertion errors https://tests.stockfishchess.org/actions?max_actions=2&action=&user=&text=AssertionError&before=1723033368.540148&run_id= I have not debugged it yet (it is again for SPSA tests). |
Ok this is master
This is this PR
The difference between
in master vs
in this PR. The difference it that the first expression only refers to the current mini task and the second expression refers to the aggregated results. So I think the second expression should be replaced by
|
This would be the patch wrt the current PR. I am now testing it diff --git a/worker/games.py b/worker/games.py
index cc82c02..9ef2796 100644
--- a/worker/games.py
+++ b/worker/games.py
@@ -990,15 +990,7 @@ def parse_fastchess_output(
spsa["losses"] = fastchess_WLD_results["losses"]
spsa["draws"] = fastchess_WLD_results["draws"]
- num_games_finished = (
- fastchess_WLD_results["games"]
- + saved_stats["wins"]
- + saved_stats["losses"]
- + saved_stats["draws"]
- )
-
- fastchess_ptnml_results = None
- fastchess_WLD_results = None
+ num_games_finished = fastchess_WLD_results["games"]
assert (
2 * sum(result["stats"]["pentanomial"])
@@ -1006,10 +998,13 @@ def parse_fastchess_output(
+ result["stats"]["losses"]
+ result["stats"]["draws"]
)
- assert num_games_finished == 2 * sum(result["stats"]["pentanomial"])
+ assert num_games_finished == 2 * sum(fastchess_ptnml_results)
assert num_games_finished <= num_games_updated + batch_size
assert num_games_finished <= games_to_play
+ fastchess_ptnml_results = None
+ fastchess_WLD_results = None
+
# Send an update_task request after a batch is full or if we have played all games.
if (num_games_finished == num_games_updated + batch_size) or (
num_games_finished == games_to_play |
it seems i found out what went wrong, fastchess when encountering time losses displays a warning to the user like below:
this warning does not exist on cutechess. |
Updated fast-chess after the fix, thanks for analysis and fixes. |
# Parse line like this:
# Finished game 1 (stockfish vs base): 0-1 {White disconnects}
if "disconnects" in line or "connection stalls" in line:
result["stats"]["crashes"] += 1
if "on time" in line:
result["stats"]["time_losses"] += 1 i suggest changing to: # Parse line like this:
# Finished game 1 (stockfish vs base): 0-1 {White disconnects}
if "disconnect" in line or "stall" in line:
result["stats"]["crashes"] += 1
if "on time" in line or "timeout" in line:
result["stats"]["time_losses"] += 1 might also want to rename timelosses to timeouts |
converting to draft, awaiting a change in convention for mate in plies / moves in the pgn output: Edit: Fixed, PR updated. |
"-tournament",
"gauntlet", better to get rid of this since its unecessary and fastchess currently supports only roundrobin format |
fixes #2106
This PR switches from cutechess-cli to fast-chess.
cutechess-cli has been serving us well in the past years, however, some issues have accumulated, namely the difficulty of compiling cutechess-cli, the observed timeouts at high concurrency and short TC, and e.g. slowness when indexing larger books. fast-chess https://github.com/Disservin/fast-chess has addressed these issues, and has now probably become mature enough to serve as the game manager for fishtest.
As an example of its ability to deal with short TC and high concurrency: https://dfts-0.pigazzini.it/tests/view/669249cdbee8253775cede32 with concurrency 25, and TC 1+0.01s no timeouts are observed.
fast-chess is built from sources, with the zip download as well as the binary cached as needed. There is fine-grained control over which version of fast-chess is used, so we can easily upgrade for new features.
In this PR, fast-chess is built in cutechess compatibility to facilitate integration, and to benefit from the existing fishtest checks. Once validated, we should be able to switch easily to its native mode, which can output trinomial and pentanomial results, and we should be able significantly simplify the worker's book-keeping.