Chore/add loss column - #42
Conversation
WalkthroughAdded a new REAL column Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Validator
participant ScoreDB
participant SQLite
Validator->>ScoreDB: save_raw_score(uid, raw_score)
ScoreDB->>SQLite: INSERT/UPDATE miner_scores (raw_score, ...)
SQLite-->>ScoreDB: OK
ScoreDB-->>Validator: ack
Note over Validator,ScoreDB: New step to persist raw loss
Validator->>ScoreDB: update_raw_loss(uid, eval_loss)
rect rgba(0,128,96,0.08)
ScoreDB->>SQLite: UPDATE miner_scores SET raw_loss = ? WHERE uid = ?
alt rows_updated > 0
SQLite-->>ScoreDB: OK (rows affected)
ScoreDB-->>Validator: success
else no rows updated
SQLite-->>ScoreDB: OK (0 rows)
ScoreDB-->>Validator: warning (no such uid)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
flockoff/validator/database.py (1)
158-171: Consider usinglogging.exceptionfor better error diagnostics.The implementation correctly mirrors the pattern of
update_raw_eval_score. However, usinglogging.errorin the except block (line 170) omits the stack trace. Consider usinglogging.exceptioninstead for easier debugging.Apply this diff:
except sqlite3.Error as e: - logger.error(f"Failed to update raw_loss for UID {uid}: {str(e)}") + logger.exception(f"Failed to update raw_loss for UID {uid}: {str(e)}") raise DatabaseError(f"Failed to update raw_loss: {str(e)}") from eOptional: The
update_raw_lossandupdate_raw_eval_scoremethods are nearly identical. In the future, consider extracting a generic_update_columnhelper to reduce code duplication.Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
flockoff/validator/database.py(2 hunks)neurons/validator.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
neurons/validator.py (1)
flockoff/validator/database.py (1)
update_raw_loss(158-171)
🪛 Ruff (0.14.4)
flockoff/validator/database.py
170-170: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
170-170: Use explicit conversion flag
Replace with conversion flag
(RUF010)
171-171: Avoid specifying long messages outside the exception class
(TRY003)
171-171: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (2)
neurons/validator.py (1)
503-504: Verify the need for storing the same value in two columns.Both
update_raw_eval_scoreandupdate_raw_lossare called with the identicaleval_lossvalue, resulting in redundant storage in theraw_scoreandraw_losscolumns. Unless there's a future plan for these metrics to diverge, this creates unnecessary data duplication and potential maintenance overhead.Consider clarifying:
- Is there a semantic difference intended between
raw_scoreandraw_loss?- Will these values be computed differently in future updates?
- If not, should one of these calls be removed?
flockoff/validator/database.py (1)
42-42: LGTM!The column addition follows the existing pattern using
_add_column_if_not_exists, ensuring backward compatibility with existing databases.
| c.execute("PRAGMA table_info(miner_scores)") | ||
| columns = {row[1] for row in c.fetchall()} | ||
| expected_columns = {"uid", "hotkey", "raw_score", "normalized_score", "namespace", "revision"} | ||
| expected_columns = {"uid", "hotkey", "raw_score", "normalized_score", "namespace", "revision", "raw_loss"} |
There was a problem hiding this comment.
Add test coverage for the new raw_loss functionality.
While the schema validation is correctly updated, there are no tests for the new update_raw_loss method mentioned in the PR summary. Following the existing test pattern in this file (e.g., test_update_raw_eval_score, test_get_raw_eval_score), you should add:
test_update_raw_loss- to verify updating raw_loss for UIDstest_get_raw_loss- if a getter method exists
Would you like me to generate test cases following the established patterns in this file?
🤖 Prompt for AI Agents
In tests/FlockDataset/validators/test_database.py around line 21, the test suite
added the new raw_loss column but lacks tests for the new update_raw_loss/getter
behavior; add two tests mirroring existing patterns: (1) test_update_raw_loss —
create a test dataset/fixture, call the dataset.update_raw_loss(uid, value) for
one or more UIDs, then query the DB (or use existing helper like
get_raw_eval_score pattern) to assert the raw_loss column was updated to
expected values and that other rows remained unchanged; (2) test_get_raw_loss —
insert known raw_loss values (or use update_raw_loss), call the
dataset.get_raw_loss(uid) getter and assert it returns the expected numeric
values and handles missing UIDs appropriately; use the same fixtures,
setup/teardown and assertion styles as
test_update_raw_eval_score/test_get_raw_eval_score in this file.
2 participants
add raw loss column
Summary by CodeRabbit
New Features
Tests