-
Notifications
You must be signed in to change notification settings - Fork 25
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
Temporary scoring formula #368
Conversation
It takes organic jobs and properly rejected synthetic jobs into account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't include dancing because we've talked about it after this PR was submitted, right?
Adding the organic jobs to the scoring was implemented in a wrong way, that messed up executor class weights normalization. :(
…mputeHorde into tmp-scoring-formula
…mputeHorde into tmp-scoring-formula
batch_scores[hotkey] += score | ||
|
||
# apply manifest bonus | ||
previous_batch = SyntheticJobBatch.objects.order_by("-id").exclude(id=batch.id).first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see it was like that before, meaning that "previous batch" was determined based on id order, but it somehow hurts me terribly. Do you know why it's based on id? We have block numbers, we can search for a batch from the actual previous cycle. Taking the largest id might yield unexpected results if a vali skips a cycle. What should happen then? i think it should be assumed there is no previous batch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why it's based on id?
🤷♂️
What should happen then? i think it should be assumed there is no previous batch.
Yeah, that sounds about right. When validators miss a batch, they should apply bonus on the next batch. I'll adjust it.
) | ||
|
||
scores = score_batches([batch]) | ||
assert scores.get("hotkey1", 0) > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these calculations tested elsewhere? Because this assertion will not detect unintended changes in the calculation algorithm, i think there should be a == ...
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added test_temporary_scoring_formula
which checks the formula for 1 executor class. IDK if the same test should also check executor class weights or not. There's already a test thats only checks for class weights, but not excused/organic jobs (test_score_batches_executor_classes_weights
). I'm not sure if checking both together is necessary or not.
validator/app/src/compute_horde_validator/validator/tests/test_utils.py
Outdated
Show resolved
Hide resolved
…mputeHorde into tmp-scoring-formula
No description provided.