Added worker-id commandline parameter #2655
Conversation
luigi/worker.py
Outdated
| @@ -407,6 +407,8 @@ def check_complete(task, out_queue): | |||
| class worker(Config): | |||
| # NOTE: `section.config-variable` in the config_path argument is deprecated in favor of `worker.config_variable` | |||
|
|
|||
| id = Parameter(default=None, | |||
There was a problem hiding this comment.
Parameters should have a default of ''. If you truly want default None, consider OptionalParameter
There was a problem hiding this comment.
Nice catch @dlstadther, but for this case certainly use '' and just do if on it.
There was a problem hiding this comment.
Changing this to '' should be good enough. We are already doing _config.id or anyway.
luigi/worker.py
Outdated
| self._config = worker(**kwargs) | ||
|
|
||
| worker_id = worker_id or self._config.id or 'Worker(%s)' % ', '.join(['%s=%s' % (k, v) for k, v in self._worker_info]) |
There was a problem hiding this comment.
since you've already modified this line, any objection to going ahead and replacing that ugly % string interpolation with .format()? :)
There was a problem hiding this comment.
I would also not do a one liner for this, so it's more readable
luigi/worker.py
Outdated
| @@ -407,6 +407,8 @@ def check_complete(task, out_queue): | |||
| class worker(Config): | |||
| # NOTE: `section.config-variable` in the config_path argument is deprecated in favor of `worker.config_variable` | |||
|
|
|||
| id = Parameter(default=None, | |||
There was a problem hiding this comment.
Nice catch @dlstadther, but for this case certainly use '' and just do if on it.
luigi/worker.py
Outdated
| self._config = worker(**kwargs) | ||
|
|
||
| worker_id = worker_id or self._config.id or 'Worker(%s)' % ', '.join(['%s=%s' % (k, v) for k, v in self._worker_info]) |
There was a problem hiding this comment.
I would also not do a one liner for this, so it's more readable
| @@ -625,6 +626,10 @@ def _generate_worker_info(self): | |||
| pass | |||
| return args | |||
|
|
|||
| def _generate_worker_id(self, worker_info): | |||
| worker_info_str = ', '.join(['{}={}'.format(k, v) for k, v in worker_info]) | |||
There was a problem hiding this comment.
I don't think format looks any less uglier. f-strings would be a different matter but i don't think they're supported on older python.
There was a problem hiding this comment.
We promote format since it's more pythonic. It's implemented without magic unlike %. Luigi only supports 2.7+ (other versions are long deprecated)
| @@ -625,6 +626,10 @@ def _generate_worker_info(self): | |||
| pass | |||
| return args | |||
|
|
|||
| def _generate_worker_id(self, worker_info): | |||
| worker_info_str = ', '.join(['{}={}'.format(k, v) for k, v in worker_info]) | |||
There was a problem hiding this comment.
We promote format since it's more pythonic. It's implemented without magic unlike %. Luigi only supports 2.7+ (other versions are long deprecated)
|
Can you fix the merge conflict? Sorry about this, I would do it in the browser but I'm afraid I'll break the line length limit if I do it in the browser. |
|
@Tarrasch Done. You can merge it in. |
|
Looks like this is ready! Merging |
Re-opening #2267 with a rebased branch.
Description
This change adds a
--worker-idwhich will be used for worker to identify itself. The user is expected to make sure these are unique.Motivation and Context
We needed to start and (safely) shutdown workers based on resource availability. In order to use
disable-workerrpc call we needed the worker id. But there is no way to know the same. We can't rely on the included host name since we are using ECS to run the workers inside containersThis change allows us to set the worker-id using a known uuid and other details and use the same to disable it later.
Have you tested this? If so, how?
This is already available as a function parameter (which doesn't seem to be used anywhere). We tested it in our own setup without any issues.