Skip to content

feat: Extract Common Configs + Log Configs on Init + Add test_ to sdk/tests filenames required for pytest - #434

Merged
ishandhanani merged 51 commits into
ai-dynamo:mainfrom
jon-chuang:jon-chuang/extract-common-config
Apr 9, 2025
Merged

ishandhanani merged 51 commits into
ai-dynamo:mainfrom
jon-chuang:jon-chuang/extract-common-config

Conversation

@jon-chuang

@jon-chuang jon-chuang commented Mar 29, 2025 •

Copy link
Copy Markdown
Contributor

Overview:

Extracts common config

Details:

  1. Makes it much less likely that the user misconfigures - e.g. it becomes much harder to set block-size to be two different values in two different components
  2. Reduces churn when updating configs - only a single field needs to be updated for the change to be applied uniformly

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

Addresses:

CC @ryanolson


Validated working in my single node env

dynamo serve graphs.agg:Frontend -f ./configs/agg.yaml
2025-03-29T21:01:37+0000 [WARNING] [cli] Converting 'Frontend' to lowercase: 'frontend'.
INFO 03-29 21:01:39 __init__.py:190] Automatically detected platform cuda.
WARNING 03-29 21:01:39 nixl.py:18] NIXL is not available
2025-03-29T21:01:40+0000 [WARNING] [cli] Converting 'Frontend' to lowercase: 'frontend'.
2025-03-29T21:01:40+0000 [INFO] [cli] Starting graphs.agg:Frontend (Press CTRL+C to quit)
INFO 03-29 21:01:43 __init__.py:190] Automatically detected platform cuda.
INFO 03-29 21:01:43 __init__.py:190] Automatically detected platform cuda.
INFO 03-29 21:01:43 __init__.py:190] Automatically detected platform cuda.
WARNING 03-29 21:01:44 nixl.py:18] NIXL is not available
WARNING 03-29 21:01:44 nixl.py:18] NIXL is not available
WARNING 03-29 21:01:44 nixl.py:18] NIXL is not available
Using default Dynamo address: dynamo/Processor
Using default Dynamo address: dynamo/VllmWorker
chat model deepseek-ai/DeepSeek-R1-Distill-Llama-8B removed from the public namespace: public
Added new chat model deepseek-ai/DeepSeek-R1-Distill-Llama-8B
+------------+------------------------------------------+-----------+-----------+------------------+
| MODEL TYPE | MODEL NAME                               | NAMESPACE | COMPONENT | ENDPOINT         |
+------------+------------------------------------------+-----------+-----------+------------------+
| chat       | deepseek-ai/DeepSeek-R1-Distill-Llama-8B | dynamo    | Processor | chat/completions |
+------------+------------------------------------------+-----------+-----------+------------------+
Starting HTTP server
2025-03-29T21:01:46.172235Z  INFO dynamo_runtime::pipeline::network::tcp::server: tcp transport service on 192.168.48.118:33909
2025-03-29T21:01:46.172791Z  INFO dynamo_llm::http::service::discovery: added Chat model: deepseek-ai/DeepSeek-R1-Distill-Llama-8B
2025-03-29T21:01:46.172786Z  INFO dynamo_llm::http::service::service_v2: Starting HTTP service on: 0.0.0.0:8000 address="0.0.0.0:8000"

Tests now run successfully in CI
image

@copy-pr-bot

copy-pr-bot Bot commented Mar 29, 2025

Copy link
Copy Markdown

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.

@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi jon-chuang! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@github-actions github-actions Bot added the external-contribution Pull request is from an external contributor label Mar 29, 2025
@jon-chuang

jon-chuang commented Apr 8, 2025 •

Copy link
Copy Markdown
Contributor Author

Can we turn this into a pytest that can run in CI?

I believe this is already run by pytest?

image
Though I find it suspicious that there is only 1 dot associated with it - I think pytest is not actually running in CI 🤦

@jon-chuang

Copy link
Copy Markdown
Contributor Author

FYI @ishandhanani
#556

@nnshah1

nnshah1 commented Apr 9, 2025

Copy link
Copy Markdown
Contributor

Can we turn this into a pytest that can run in CI?

I believe this is already run by pytest?

image Though I find it suspicious that there is only 1 dot associated with it - I think pytest is not actually running in CI 🤦

there is two aspects of pytest in ci - one is to run mypy - that runs on all python files

the other would be what is included with a pyest mark of pre-merge - we can help here

@nnshah1

nnshah1 commented Apr 9, 2025

Copy link
Copy Markdown
Contributor

FYI @ishandhanani #556

put a comment on other issue as well - but agree this is non obvious - if the test doesn't run after putting the mark let us know - and we'll be updating a testing strategy over the next weeks to make it clearer what is run when.

Comment thread deploy/dynamo/sdk/src/dynamo/sdk/tests/config.py
@jon-chuang jon-chuang changed the title feat: Extract Common Configs + Log Configs on Init feat: Extract Common Configs + Log Configs on Init + Fix test_ file prefix required for pytest Apr 9, 2025
@jon-chuang jon-chuang changed the title feat: Extract Common Configs + Log Configs on Init + Fix test_ file prefix required for pytest feat: Extract Common Configs + Log Configs on Init + Add test_ to several test filenames required for pytest Apr 9, 2025
@jon-chuang jon-chuang changed the title feat: Extract Common Configs + Log Configs on Init + Add test_ to several test filenames required for pytest feat: Extract Common Configs + Log Configs on Init + Add test_ to sdk test filenames required for pytest Apr 9, 2025
@jon-chuang jon-chuang changed the title feat: Extract Common Configs + Log Configs on Init + Add test_ to sdk test filenames required for pytest feat: Extract Common Configs + Log Configs on Init + Add test_ to sdk/tests filenames required for pytest Apr 9, 2025
@ishandhanani

Copy link
Copy Markdown
Contributor

This last batch of commit messages are great @jon-chuang 😆

@mohammedabdulwahhab mohammedabdulwahhab left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@ishandhanani

Copy link
Copy Markdown
Contributor

Thanks for this @jon-chuang - merging to main

@ishandhanani
ishandhanani merged commit ba2727d into ai-dynamo:main Apr 9, 2025
kylehh pushed a commit to kylehh/dynamo that referenced this pull request Apr 11, 2025
…sdk/tests` filenames required for pytest (ai-dynamo#434)

Co-authored-by: ishandhanani <82981111+ishandhanani@users.noreply.github.com>
oandreeva-nv pushed a commit that referenced this pull request Apr 22, 2025
…sdk/tests` filenames required for pytest (#434)

Co-authored-by: ishandhanani <82981111+ishandhanani@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contribution Pull request is from an external contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants