-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/inherit config support #39
Conversation
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.
it's better if kannon doesn't depend TASK_WORKSPACE_DIRECTORY
which is not required env var on gokart.
On the other hand, kannon already has some code based on TASK_WORKSPACE_DIRECTORY
so I think it's ok to rely on it for now.
But I would like to consider about workspace_directory
design consistency with gokart.
We replace Requiring Note that users are still required to specify remote workspace directory info via |
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.
LGTM!
@@ -56,6 +55,26 @@ def __init__( | |||
self.task_id_to_job_name: dict[str, str] = dict() | |||
|
|||
def build(self, root_task: gokart.TaskOnKart) -> None: | |||
# TODO: support multiple dynamic config 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.
If you like to support this, I think it is better to change the parameter name plural form ( dynamic_config_paths
) for compatibility. And then you assert like following
assert len(dynamic_config_paths) == 1, 'Currently kannon doesn't support multiple dynamic config files'
Support a config file generated dynamically.
In a basic usage, we have all config files to be registered to luigi initially when starting docker container from an image, since we usually copy these files to container local.
However, in some advanced cases, we generate additional config files dynamically after starting docker container.
This kind of file is not specified in Dockerfile, so when starting child jobs, they do not have this file and failed when utilizing the dynamically generated config files.
So, this PR adds a feature where the master job passes the config info to children via gokart cache.
Currently, only one file is supported, but in the future we'll support multiple dynamic config files / other format of files.