Skip to content
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

SUBMARINE-942. Make experiment ID consistent with TFJob and PyTorch Job #683

Closed
wants to merge 7 commits into from

Conversation

kevin85421
Copy link
Member

@kevin85421 kevin85421 commented Jul 17, 2021

What is this PR for?

  1. Please refer to the following two JIRA issues.

    In this JIRA issue, we need to make the experiment ID consistent with the name of TFJob and PyTorch Job. The difference is caused by this link. Update ExperimentId.java.

  2. Update IntegrationTestK8s.md

    • "submarine-server-core" is a dependency package specified in pom.xml of test-k8s.
    • In the document BuildFromCode.md, the command to build Submarine is mvn clean package -DskipTests. However, the package will be installed into the local repository at install phase, a later phase than both package and verify phases.
    • Hence, we need to execute mvn install -DskipTests to ensure that the test-k8s module uses the latest "submarine-server-core" module.
  3. Remove the field name in Experiment.java because the value of name is the same as experimentId.

What type of PR is it?

[Improvement]

Todos

What is the Jira issue?

https://issues.apache.org/jira/browse/SUBMARINE-942

How should this be tested?

Screenshots (if appropriate)

Questions:

  • Do the license files need updating? No
  • Are there breaking changes for older versions? No
  • Does this need new documentation? No

@kevin85421 kevin85421 marked this pull request as draft July 17, 2021 16:25
@kevin85421
Copy link
Member Author

kevin85421 commented Jul 17, 2021

@pingsutw and I are trying to figure out the root cause of a weird problem about test-k8s. Hence, this PR is not ready to be reviewed. Thanks!

Update: the root cause of the weird problem is written in the "What is this PR for?".

@kevin85421 kevin85421 marked this pull request as ready for review July 19, 2021 08:21
@kevin85421
Copy link
Member Author

@jiwq Can you help me review this PR? Thanks!
@pingsutw Can you help me review the update about IntegrationTestK8s.md in this PR? Thanks!

@kevin85421
Copy link
Member Author

@pingsutw I have removed the field name in Experiment.java. Thanks!

@asfgit asfgit closed this in 6105358 Jul 25, 2021
@pingsutw pingsutw reopened this Jul 25, 2021
@pingsutw
Copy link
Member

Reopen it since some tests failed.

@kevin85421
Copy link
Member Author

kevin85421 commented Jul 25, 2021

@pingsutw In my opinion, the root cause of the CI error is caused by SUBMARINE-946 rather than SUBMARINE-942. The error message indicates that the package does not be loaded into the maven repo. If needed, I can try to fix the error.

@pingsutw
Copy link
Member

But tests passed after I revert SUBMARINE-942.
https://github.com/apache/submarine/runs/3155487485

@kevin85421
Copy link
Member Author

kevin85421 commented Jul 25, 2021

@pingsutw This is because other pull requests do not change the submarine-server-core module. I will rebase the merged commits to reproduce the CI failures.

@asfgit asfgit closed this in 9292943 Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants