Fix stearming synthetic #390
Conversation
mpnowacki-reef
force-pushed
the
fix-stearming-synthetic
branch
from
8634f0e to
36911b6
Compare
Validator test report160 tests ±0 158 ✅ ±0 42s ⏱️ ±0s Results for commit 9203142. ± Comparison against base commit 940f094. This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
mpnowacki-reef
force-pushed
the
fix-stearming-synthetic
branch
from
42adb29 to
872572a
Compare
mpnowacki-reef
force-pushed
the
fix-stearming-synthetic
branch
from
872572a to
9203142
Compare
| miner.pk, | ||
| SyntheticJob.Status.FAILED, | ||
| 0, | ||
| re.compile("took too long: time_took_sec=.*"), |
There was a problem hiding this comment.
I understand this is because of the "sleepy request" in the prep method?
I'd move that setup to this test, if possible, IMO it'll be easier to understand.
There was a problem hiding this comment.
you mean to place the body of the prep method inside of the test method? I'm thinking maybe the per miner configuration should be more explicit and be sth like
[
Miner(hotkey=..., ws_messages='all', http_responses='ok'),
Miner(hotkey=..., ws_messages='up_until_ready', http_responses='ok'),
Miner(hotkey=..., ws_messages='all', http_responses='timeout'),
...
]
which would make it even more readable and would avoid the reader to have bind associate a miner it's http mock config via port numbers and other nonsense. WDYT?
There was a problem hiding this comment.
Yes, I didn't necessarily mean all of it, but the "sleepy request" part.
It's that I had to go to the fixture to see how many uuids there are, then calculate the port number and then go to that function to see why job uuid[-2] is supposed to time out – having the setup in the test would make this easier, I think.
Your suggestion from this comment is even better, of course, if you think it's worth to spend time doing that.
| except Exception as e: | ||
| msg = f"Failed to execute streaming job {job_uuid} on {url}: {e}" | ||
| logger.debug(msg) | ||
| else: |
There was a problem hiding this comment.
I think early returns are easier to follow than try/except/else, if they're possible. I think we can do that in this case – we don't need to move further and request to /terminate the job that hasn't been successfully started, right?
Same for below
There was a problem hiding this comment.
i think we do need to terminate, or at least try
| # TODO: caching | ||
| val = await aget_config("DYNAMIC_SYNTHETIC_STREAMING_JOB_READY_TIMEOUT") | ||
| return float(val) if val is not None else None | ||
|
|
||
|
|
||
| class LlmPromptsSyntheticJobGenerator(LlmPromptsJobGenerator): |
There was a problem hiding this comment.
Note, maybe out of scope: the naming of these classes is not clear to me (BaseSyntheticJobGenerator -> LlmPromptsJobGenerator -> LlmPromptsSyntheticJobGenerator). Could the last one be called something with "streaming" instead, maybe?
| @@ -37,6 +48,7 @@ def __init__( | |||
| self.s3_output_bucket = settings.S3_BUCKET_NAME_ANSWERS | |||
|
|
|||
| self.prompt_answers: dict[str, str] = {} | |||
| self.streaming_processing_time: float | None = None | |||
There was a problem hiding this comment.
I think this can be moved to the subclass, it'd be cleaner this way
| return True | ||
|
|
||
| def verify_correctness(self, msg: V0JobFinishedRequest) -> tuple[bool, str]: | ||
| return True, "mock" |
There was a problem hiding this comment.
I think verify can be removed from this class now, and the TimeTookScoreMockSyntheticJobGenerator class and factory seem to no longer be not needed at all
No description provided.