-
Notifications
You must be signed in to change notification settings - Fork 757
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
fix(scheduling): numpy worker environs are not taking effect #2893
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2893 +/- ##
==========================================
- Coverage 70.88% 69.35% -1.54%
==========================================
Files 103 103
Lines 9335 9374 +39
==========================================
- Hits 6617 6501 -116
- Misses 2718 2873 +155
|
@@ -51,20 +58,10 @@ def main( | |||
- file:///path/to/unix.sock | |||
- fd://12 | |||
working_dir: (Optional) the working directory | |||
worker_id: (Optional) if set, the runner will be started as a worker with the given ID | |||
worker_id: (Optional) if set, the runner will be started as a worker with the given ID. Important: begin from 1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add arg doc for worker_env_map
required=False, | ||
type=click.STRING, | ||
default=None, | ||
help="The environment variables to pass to the worker process. The format is a JSON string, e.g. '{0: {\"CUDA_VISIBLE_DEVICES\": 0}}'.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use a dotenv file instead of JSON? that seems easier for debugging purpose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The env map includes all envvars for each worker.
{
0: {"CUDA_VISIBLE_DEVICES": 0},
1: {"CUDA_VISIBLE_DEVICES": 1},
}
It seems not that easy to be represented by dotenv files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that passing JSON in CLI isn't the most intuitive. Using an .env
file, we can allow multiple arguments of key-value pairs.
--worker-env 0:worker_0.env --worker-env 1:worker_1.env
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ssheng I think that's too complicated. This is not the public API.
The public API is the bentoml serve
and bentoml.serve
worker_id, | ||
) | ||
@property | ||
def scheduled_worker_env_map(self) -> dict[int, dict[str, t.Any]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does Yatai need this information for scheduling runners?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the worker concept is transparent for yatai
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bojiang got it, in the case of Yatai, it will just use resources available from system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah
6a26765
to
f0b7386
Compare
solution: * only allow scheduling_strategy to control the env variables * setting up environ before importing bentoml
f0b7386
to
7030027
Compare
solution:
WARNING:
a breaking change for a not documented public API
custom scheduler
fix: #2787