[
https://issues.apache.org/jira/browse/SUBMARINE-54?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16822871#comment-16822871
]
Zhankun Tang commented on SUBMARINE-54:
---------------------------------------
[~snemeth], Thanks for the patch. It looks good to me. Some comments:
1. In TensorFlowPsLaunchCommand and TensorFlowPsLaunchCommand's class comment,
"Tensorboard" seems should be "Tensorflow"?
{code:java}
/**
* Launch command implementation for Tensorboard's PS component.
*/{code}
2. In Localizer.java, the "YARN_CONTAINER_RUNTIME_DOCKER_MOUNTS" seems can be
replaced with the constant string "ENV_DOCKER_MOUNTS_FOR_CONTAINER_RUNTIME"
defined in EnvironmentUtilities.java
{code:java}
appendToEnv(service, "YARN_CONTAINER_RUNTIME_DOCKER_MOUNTS",
mountStr, ",");
{code}
3. For the "serviceWrapper.java", it's only for the test code but is used in
the main source code. I feel a little uncomfortable about this intrusive way.
Is it feasible to store the mapping and service instance in "CliContext" and
then get the mapping from CliContext when testing? If it's not feasible, let's
keep current work and refactor it sometime later.
4. Did you verify running a single worker training job manually?
> Add test coverage for YarnServiceJobSubmitter and make it ready for extension
> for PyTorch
> -----------------------------------------------------------------------------------------
>
> Key: SUBMARINE-54
> URL: https://issues.apache.org/jira/browse/SUBMARINE-54
> Project: Hadoop Submarine
> Issue Type: Sub-task
> Reporter: Szilard Nemeth
> Assignee: Szilard Nemeth
> Priority: Major
> Attachments: SUBMARINE-54.001.patch, SUBMARINE-54.002.patch,
> SUBMARINE-54.003.patch, SUBMARINE-54.004.patch, SUBMARINE-54.005.patch,
> SUBMARINE-54.006.patch, SUBMARINE-54.007.patch
>
>
> This crucial class has no associated test yet. We need to improve this.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
