Skip to content

Conversation

@Liam-DeVoe
Copy link
Collaborator

Split from #154. The allocation behavior is exactly the same as master, and several things here are no-ops. I figured splitting out the architecture makes it easier to review that, and then we can review the estimators separately.

@Liam-DeVoe Liam-DeVoe requested a review from Zac-HD June 19, 2025 07:13
Copy link
Owner

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

(better to send the review I have, than spend days more crafting a better one. feel free to push back if something seems wrong.)

Comment on lines 350 to 353
# TODO actually defer starting up targets here, based on worker lifetime
# and startup cost estimators here
for nodeid in candidates:
self.add_target(nodeid)
Copy link
Owner

Choose a reason for hiding this comment

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

I think I'd like to move this responsibility into the hub process; it can slowly 'feed in' extra targets to amortize startup cost, etc. That way only one process needs to read from the database to get good performance, and we get a pretty nice separation of concerns too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good call, I think. My only wonder is if the workers have access to expensive-to-serialize information that means they could make better decisions than the hub, but I don't think so. It's just estimator state that we're already sharing anyway.

Copy link
Owner

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Looks solid, though there are obviously some substantial followup PRs to come before it really pays off 🙂

For this PR, I'd love to have some basic tests to act as sanity-checks when we later start complicating things, and then merge.

@Liam-DeVoe Liam-DeVoe merged commit b1b1c39 into master Jun 28, 2025
13 checks passed
@Liam-DeVoe Liam-DeVoe deleted the allocate-initial-structure branch June 28, 2025 04:49
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