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

cli: split fetch-recent-miner-measurements and evaluate-measurements #401

Merged
merged 5 commits into from
Nov 14, 2024

Conversation

juliangruber
Copy link
Member

@juliangruber juliangruber commented Nov 12, 2024

Usage:

$ node bin/fetch-recent-miner-measurements.js f0...
...
$ node bin/evaluate-measurements measurements-f0....ndjson
 → evaluating round 19117n
 → added 183 accepted measurements from this round
 → evaluating round 19118n
 → added 0 accepted measurements from this round
 → evaluating round 19127n
 → added 181 accepted measurements from this round
 → evaluating round 19128n
 → added 0 accepted measurements from this round
Found 364 accepted measurements.
  IPNI_ERROR_404                           364        (100%)
Wrote evaluation to evaluation-5bf6db8.txt

This is a first step towards evaluating arbitrary rounds. Next: #402

  • After merging, update Spark troubleshooting docs

@bajtos
Copy link
Member

bajtos commented Nov 13, 2024

I'll review this PR next week. I am afraid there will be a merge conflict with #396 😢

How should we proceed to handle that in the least painful way?

@bajtos
Copy link
Member

bajtos commented Nov 13, 2024

Before your change, the default behaviour is to not show rejected measurements in the output file. IIUC the proposed version, the first step will produce an intermediate file that includes rejected measurements too. I think this can create confusion when people not familiar with the concept of accepted/rejected scores use these tools.

An idea to consider:

  • The first step creates a file with a suffix like -raw or -all
  • The second step either creates files with no suffix (i.e. preserve the current output filenames) or use a different suffix like -accepted or -evaluated.

WDYT?

@juliangruber
Copy link
Member Author

Before your change, the default behaviour is to not show rejected measurements in the output file. IIUC the proposed version, the first step will produce an intermediate file that includes rejected measurements too. I think this can create confusion when people not familiar with the concept of accepted/rejected scores use these tools.

An idea to consider:

  • The first step creates a file with a suffix like -raw or -all
  • The second step either creates files with no suffix (i.e. preserve the current output filenames) or use a different suffix like -accepted or -evaluated.

WDYT?

Since e2b5663, the output file will have .evaluation.txt suffix, which I think is great. I don't think there's any expectation about the shape of the intermediary file, it is supposed to contain all measurements submitted (in my mental model) and that it does.

@juliangruber
Copy link
Member Author

I'll review this PR next week. I am afraid there will be a merge conflict with #396 😢

How should we proceed to handle that in the least painful way?

The merge conflict isn't going to be hard to deal with if we reapply https://github.com/filecoin-station/spark-evaluate/pull/396/files on top of this PR. Therefore I propose to merge this one first.

@bajtos
Copy link
Member

bajtos commented Nov 14, 2024

I'll review this PR next week. I am afraid there will be a merge conflict with #396 😢
How should we proceed to handle that in the least painful way?

The merge conflict isn't going to be hard to deal with if we reapply https://github.com/filecoin-station/spark-evaluate/pull/396/files on top of this PR. Therefore I propose to merge this one first.

I was thinking about this some more, and would like to rework #396 - see #396 (comment)

I agree to land this pull request first.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

I quickly skimmed through the changes and I don't see any obvious problems. Let's land this and then fix any issues later as we discover them.

:shipit:

@bajtos
Copy link
Member

bajtos commented Nov 14, 2024

Before your change, the default behaviour is to not show rejected measurements in the output file. IIUC the proposed version, the first step will produce an intermediate file that includes rejected measurements too. I think this can create confusion when people not familiar with the concept of accepted/rejected scores use these tools.
An idea to consider:

  • The first step creates a file with a suffix like -raw or -all
  • The second step either creates files with no suffix (i.e. preserve the current output filenames) or use a different suffix like -accepted or -evaluated.

WDYT?

Since e2b5663, the output file will have .evaluation.txt suffix, which I think is great. I don't think there's any expectation about the shape of the intermediary file, it is supposed to contain all measurements submitted (in my mental model) and that it does.

I believe the old version included evaluation results in the measurements written to the per-miner file. After your change, the first script that fetches measurements produces a file that does not have information about the evaluation result. The second script produces only TXT file, so there is no way how to further analyse the processed per-miner measurements using jq or other tools.

Please correct me if I'm wrong.

I think this can be also iterated on in a follow-up pull request.

Please remember to update our docs (https://docs.filspark.com/troubleshooting-miner-score, the source is in Notion) after you land this change.

@juliangruber
Copy link
Member Author

I believe the old version included evaluation results in the measurements written to the per-miner file. After your change, the first script that fetches measurements produces a file that does not have information about the evaluation result. The second script produces only TXT file, so there is no way how to further analyse the processed per-miner measurements using jq or other tools.

Please correct me if I'm wrong.

Now, the measurements fetching prints a summary to stdout and produces a .ndjson file of raw measurements.

The evaluation script prints a summary to stdout and produces a .txt file of per-measurement evaluation. I will add a .ndjson file so that this data is machine processable.

@juliangruber juliangruber merged commit 9564c8c into main Nov 14, 2024
6 checks passed
@juliangruber juliangruber deleted the refactor/evaluation-scripts branch November 14, 2024 09:10
@juliangruber
Copy link
Member Author

Please remember to update our docs (https://docs.filspark.com/troubleshooting-miner-score, the source is in Notion) after you land this change.

Updated in https://www.notion.so/spacemeridian/Troubleshooting-Miner-Score-664ccb2e5c264b39986df09db6b445a4 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants