Pickling: respawning #572
Pickling: respawning #572
Conversation
yt/python/yt/wrapper/py_wrapper.py
Outdated
| def run(self): | ||
| command = self.make_command() | ||
| logger.info("Start respawn in docker") | ||
| logger.info("Command: " + " ".join(command)) |
There was a problem hiding this comment.
Let merge this message with previous one:
logger.info("Respawn in docker (command: ...)")
yt/python/yt/wrapper/py_wrapper.py
Outdated
| stdout=sys.stdout, | ||
| stderr=sys.stderr, | ||
| bufsize=1, | ||
| universal_newlines=True, |
There was a problem hiding this comment.
What the idea is behind specifying these options?
bufsize=1,
universal_newlines=True,
There was a problem hiding this comment.
This is an old artifact from the time I tried to process stdout/stderr.
I'll just delete it.
yt/python/yt/wrapper/py_wrapper.py
Outdated
| self._env = { | ||
| key: value | ||
| for key, value in os.environ.items() | ||
| if key.startswith('YT_') |
There was a problem hiding this comment.
We usually use double quotes
yt/python/yt/wrapper/py_wrapper.py
Outdated
| self._main_script_path = main_scipt_path if main_scipt_path is not None else os.path.abspath(sys.argv[0]) | ||
| self._cwd = cwd if cwd is not None else os.path.abspath(os.getcwd()) | ||
| self._homedir = homedir if homedir is not None else os.path.expanduser("~") | ||
| # we need to provide all python libraries to a container to ensure picking works |
yt/python/yt/wrapper/py_wrapper.py
Outdated
|
|
||
| def _make_docker_env_args(self, env): | ||
| return list(itertools.chain(*[ | ||
| ("-e", "{0}={1}".format(key, value)) |
There was a problem hiding this comment.
Let use chain.from_iterable here and in other similar situations.
yt/python/yt/wrapper/py_wrapper.py
Outdated
|
|
||
| def _make_docker_env_args(self, env): | ||
| return list(itertools.chain(*[ | ||
| ("-e", "{0}={1}".format(key, value)) |
There was a problem hiding this comment.
Is it safe to pass env variables as "{0}={1}"? What if it contains white space or some other special characters?
There was a problem hiding this comment.
It is a very good point.
There is a similar problem not only with environment variables, but also with other values (e.g. mount points).
So I'll add escaping for the generated command.
yt/python/yt/wrapper/py_wrapper.py
Outdated
| return self._mount | ||
| # if we mount the entire local file system in docker under an arbitrary name | ||
| # we will break the symlinks, | ||
| # so we have to find only the paths necessary for work and mount them along the same paths |
There was a problem hiding this comment.
Let fix style of comment
yt/python/yt/wrapper/py_wrapper.py
Outdated
| ])) | ||
|
|
||
| def _make_mount_paths(self): | ||
| # use user provided values |
There was a problem hiding this comment.
Let drop this comment, in my opinion it gives no additional information that is not expressed in the code
thenno
force-pushed
the
respawn_wrapper_in_docker
branch
from
31e5a33
to
95cfd8c
Compare
|
@Kontakter has imported your pull request. If you are a Yandex employee, you can view this diff. |
|
|
||
|
|
||
| @authors("thenno") | ||
| def test_docker_respawner(monkeypatch): |
There was a problem hiding this comment.
Let add pytest.skip() for case of ya make
|
|
||
| from .conftest import authors | ||
|
|
||
| from yt.wrapper.py_wrapper import DockerRespawner, respawn_in_docker |
There was a problem hiding this comment.
Let use import from yt.wrapper
| "-e", "YT_BASE_LAYER=some_image", | ||
| "-v", "/home/user:/home/user", # user's homedir | ||
| "-v", "/home/user2/yt:/home/user2/yt", # main script's dir | ||
| "-v", "/root:/root", # current cwd |
There was a problem hiding this comment.
Let here (and in other places in this file) fix the comments:
- Write it on the previous line.
- Write it as
# Specifying user's homedir.
| "-v", "/home/user:/home/user", # user's homedir | ||
| "-v", "/home/user2/yt:/home/user2/yt", # main script's dir | ||
| "-v", "/root:/root", # current cwd | ||
| # a part of pythonpython outside homedir |
|
@Kontakter has imported your pull request. If you are a Yandex employee, you can view this diff. |
|
✅ This pull request is being closed because it has been successfully merged. |
resolves ytsaurus#558 --- 938e34fa99423f85bb4a7ad055efe338ae7949a7 Pull Request resolved: ytsaurus#572
resolves #558