Skip to content

chore: KV router Pythonic cleanups #324

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Mar 20, 2025
Merged

chore: KV router Pythonic cleanups #324

merged 6 commits into from
Mar 20, 2025

Conversation

PeaBrane
Copy link
Contributor

@PeaBrane PeaBrane commented Mar 20, 2025 •
edited
Loading

Overview:

  1. Use getattr instead of hasattr to check the existence of the key (worker metrics). Cleaner and avoids a double lookup. Potentially avoids a race condition in multithreaded settings.
  2. Remove an unnecessary if statement for checking worker_logits, as a guard clause is already checking it.
  3. Added a docstring for the _cost_function method

@PeaBrane PeaBrane requested a review from alec-flowers March 20, 2025 21:11
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Mar 20, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@PeaBrane PeaBrane changed the title chore: use getattr consistent with the style of the other get methods chore: KV router Pythonic cleanups Mar 20, 2025
@PeaBrane PeaBrane merged commit c448061 into main Mar 20, 2025
5 checks passed
@PeaBrane PeaBrane deleted the rupei/kv-hasattr-cleanups branch March 20, 2025 23:23
kylehh pushed a commit to kylehh/dynamo that referenced this pull request Apr 11, 2025
@PeaBrane
chore: KV router Pythonic cleanups (ai-dynamo#324)
43ff14c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tedzhouhk tedzhouhk tedzhouhk approved these changes

@alec-flowers alec-flowers Awaiting requested review from alec-flowers alec-flowers is a code owner

@hutm hutm Awaiting requested review from hutm hutm is a code owner

@biswapanda biswapanda Awaiting requested review from biswapanda biswapanda is a code owner

@ishandhanani ishandhanani Awaiting requested review from ishandhanani ishandhanani is a code owner

@julienmancuso julienmancuso Awaiting requested review from julienmancuso julienmancuso is a code owner

@hhzhang16 hhzhang16 Awaiting requested review from hhzhang16 hhzhang16 is a code owner

@nnshah1 nnshah1 Awaiting requested review from nnshah1 nnshah1 is a code owner

@tanmayv25 tanmayv25 Awaiting requested review from tanmayv25 tanmayv25 is a code owner

@piotrm-nvidia piotrm-nvidia Awaiting requested review from piotrm-nvidia piotrm-nvidia is a code owner

@ptarasiewiczNV ptarasiewiczNV Awaiting requested review from ptarasiewiczNV ptarasiewiczNV is a code owner

@ryanolson ryanolson Awaiting requested review from ryanolson ryanolson is a code owner

@grahamking grahamking Awaiting requested review from grahamking grahamking is a code owner

@paulhendricks paulhendricks Awaiting requested review from paulhendricks paulhendricks is a code owner

@tmonty12 tmonty12 Awaiting requested review from tmonty12 tmonty12 is a code owner

@GuanLuo GuanLuo Awaiting requested review from GuanLuo GuanLuo is a code owner

@rmccorm4 rmccorm4 Awaiting requested review from rmccorm4 rmccorm4 is a code owner

@kkranen kkranen Awaiting requested review from kkranen kkranen is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PeaBrane @tedzhouhk