Skip to content
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.

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

Fixed workflow store pathing issue #2792 #2797

Merged
merged 6 commits into from
Nov 20, 2023
Merged

Fixed workflow store pathing issue #2792 #2797

merged 6 commits into from
Nov 20, 2023

Conversation

udaij12
Copy link
Collaborator

@udaij12 udaij12 commented Nov 16, 2023

#2792

Issue is that workflow store pathing in config.properties is being overwritten by the default workflow store location.
Fixed this so that default path is only set if config.properties and cmd line are both empty.

Feature/Issue validation/testing:

  • Test Case 1:

    • Created test in configManagerTest suite - testWorkflowState()
    • Checks if getWorkflowStore() returns the same path as provided in config_test_env.properties
  • Test Case 2:

    • Created test in configManagerTest suite - testNoWorkflowState
    • Checks if getWorkflowStore() returns the default path (model_store path) since config_test_workflow.properties has no workflow_path
  • Test Case 3: added workflow_store path to config.properties and expect it to be used.

config.properties:

inference_address=http://0.0.0.0:8080
management_address=http://0.0.0.0:8081
metrics_address=http://0.0.0.0:8082
number_of_netty_threads=32
job_queue_size=1000
model_store=/home/ubuntu/serve/model_store
workflow_store=/home/ubuntu/serve/wf-store

Ran:
torchserve --start --ncs --ts-config /home/ubuntu/serve/config.properties

Result:

Torchserve version: 0.9.0
TS Home: /opt/conda/lib/python3.10/site-packages
Current directory: /home/ubuntu/serve
Temp directory: /tmp
Metrics config path: /opt/conda/lib/python3.10/site-packages/ts/configs/metrics.yaml
Number of GPUs: 1
Number of CPUs: 4
Max heap size: 3924 M
Python executable: /opt/conda/bin/python
Config file: /home/ubuntu/serve/config.properties
Inference address: http://0.0.0.0:8080
Management address: http://0.0.0.0:8081
Metrics address: http://0.0.0.0:8082
Model Store: /home/ubuntu/serve/model_store
Initial Models: N/A
Log dir: /home/ubuntu/serve/logs
Metrics dir: /home/ubuntu/serve/logs
Netty threads: 32
Netty client threads: 0
Default workers per model: 1
Blacklist Regex: N/A
Maximum Response Size: 6553500
Maximum Request Size: 6553500
Limit Maximum Image Pixels: true
Prefer direct buffer: false
Allowed Urls: [file://.*|http(s)?://.*]
Custom python dependency for model allowed: false
Enable metrics API: true
Metrics mode: log
Disable system metrics: false
Workflow Store: /home/ubuntu/serve/wf-store
Model config: N/A
  • Test Case 4: Config file with no workflow store and no command line input, Expect default value (model_store path)

config.properties:

inference_address=http://0.0.0.0:8080
management_address=http://0.0.0.0:8081
metrics_address=http://0.0.0.0:8082
number_of_netty_threads=32
job_queue_size=1000
model_store=/home/ubuntu/serve/model_store

Ran:

torchserve --start --ncs --model-store /home/ubuntu/serve/model_store

Result:

Torchserve version: 0.9.0
TS Home: /opt/conda/lib/python3.10/site-packages
Current directory: /home/ubuntu/serve
Temp directory: /tmp
Metrics config path: /opt/conda/lib/python3.10/site-packages/ts/configs/metrics.yaml
Number of GPUs: 1
Number of CPUs: 4
Max heap size: 3924 M
Python executable: /opt/conda/bin/python
Config file: config.properties
Inference address: http://0.0.0.0:8080
Management address: http://0.0.0.0:8081
Metrics address: http://0.0.0.0:8082
Model Store: /home/ubuntu/serve/model_store
Initial Models: N/A
Log dir: /home/ubuntu/serve/logs
Metrics dir: /home/ubuntu/serve/logs
Netty threads: 32
Netty client threads: 0
Default workers per model: 1
Blacklist Regex: N/A
Maximum Response Size: 6553500
Maximum Request Size: 6553500
Limit Maximum Image Pixels: true
Prefer direct buffer: false
Allowed Urls: [file://.*|http(s)?://.*]
Custom python dependency for model allowed: false
Enable metrics API: true
Metrics mode: log
Disable system metrics: false
Workflow Store: /home/ubuntu/serve/model_store
Model config: N/A

@udaij12 udaij12 marked this pull request as ready for review November 16, 2023 22:36
@lxning
Copy link
Collaborator

lxning commented Nov 16, 2023

could you add a test in the PR and also update Readme

@lxning
Copy link
Collaborator

lxning commented Nov 18, 2023

Could you add one test that workflow_store is set neither cmd nor config.properties?

@@ -107,6 +107,14 @@ There are no default required arguments to start the server
1. **start**: optional, A more descriptive way to start the server.
1. **stop**: optional, Stop the server if it is already running.

#### Argument Priority:
Arguments can be set in multiple locations (ex: command line, config.properties). The following is the priority:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Arguments can be set in multiple locations (ex: command line, config.properties). The following is the priority:
Arguments can be set in multiple locations (ex: command line, config.properties). The following is the priority from highest to lowest:

Also, should we include where environment variable configurations stand in this list?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Environment variables take highest precedence but enable_envvars_config has to be set true in config.properties, so it isnt enabled by default. I can add it to the doc if needed.

@lxning lxning added this pull request to the merge queue Nov 20, 2023
Merged via the queue into master with commit 796808f Nov 20, 2023
13 checks passed
@chauhang chauhang added this to the v0.10.0 milestone Feb 27, 2024
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.

4 participants