Skip to content

Comments

Use the baseline cache for the sampling evaluator#550

Merged
mtrofin merged 2 commits intomainfrom
spr/mtrofin/use-the-baseline-cache-for-the-sampling-evaluator
Feb 18, 2026
Merged

Use the baseline cache for the sampling evaluator#550
mtrofin merged 2 commits intomainfrom
spr/mtrofin/use-the-baseline-cache-for-the-sampling-evaluator

Conversation

@mtrofin
Copy link
Collaborator

@mtrofin mtrofin commented Feb 13, 2026

No description provided.

@mtrofin mtrofin requested review from boomanaiden154 and svkeerthy and removed request for svkeerthy February 13, 2026 05:31
@mtrofin mtrofin marked this pull request as draft February 13, 2026 07:01
@mtrofin mtrofin changed the base branch from spr/mtrofin/main.use-the-baseline-cache-for-the-sampling-evaluator to main February 18, 2026 01:26
@mtrofin mtrofin force-pushed the spr/mtrofin/use-the-baseline-cache-for-the-sampling-evaluator branch from dc06f36 to 5f3a0f7 Compare February 18, 2026 01:30
@mtrofin mtrofin marked this pull request as ready for review February 18, 2026 01:30
Copy link
Collaborator

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, one question, one nit.

not_done, return_when=concurrent.futures.FIRST_COMPLETED)
return futures

def ensure_baselines(self, pool):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: naming seems a bit weird. I feel like something like compute_baselines would fit a bit better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it doesn't compute if they are computed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe get_baselines then? Either way we still setup self._baselines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

get_baselines suggests something is returned, but it's not. Also, suggests simple accessor, and also const-ness. Neither are true.

ensure says what it does.

@mtrofin mtrofin merged commit 7badae3 into main Feb 18, 2026
15 checks passed
@mtrofin mtrofin deleted the spr/mtrofin/use-the-baseline-cache-for-the-sampling-evaluator branch February 18, 2026 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants