Skip to content

Conversation

mmh683
Copy link
Contributor

@mmh683 mmh683 commented Feb 1, 2019

The idea is to unify the environment variables JOB_BASE_NAME and BUILD_ENVIRONMENT which controlled the Pytorch and Caffe2 jobs respectively. In this commit, we have converted all the JOB_BASE_NAME references in .jenkins/pytorch/* files to BUILD_ENVIRONMENT. Then, did the same thing in .circleci/config.yml. One thing that we needed to be careful was when both BUILD_ENVIRONMENT and JOB_BASE_NAME were present under same declaration in config.yml file (e.g., for "caffe2-" stuffs). To ensure that all "==" checks work as expected, we also had to add "*" in some if conditions in .jenkins/caffe2/build.sh file. Finally, removed "-build", "-test", etc. suffixes from COMPACT_JOB_NAME variable assignment in the bash script files in .jenkins/pytorch folder, e.g., modify COMPACT_JOB_NAME="${BUILD_ENVIRONMENT}-build" to COMPACT_JOB_NAME="${BUILD_ENVIRONMENT}".

@soumith soumith changed the title Making the changes related to task: T39506265 (Merge job-spec env variables of Pytorch/Caffe2 CI jobs) Merge job-spec env variables of Pytorch/Caffe2 CI jobs Feb 1, 2019
@mmh683 mmh683 force-pushed the master branch 3 times, most recently from aa3ad7a to 37956ab Compare February 1, 2019 20:17
@pjh5
Copy link
Contributor

pjh5 commented Feb 1, 2019

…iables of Pytorch/Caffe2 CI jobs). The idea is to unify the environment variables JOB_BASE_NAME and BUILD_ENVIRONMENT which controlled the Pytorch and Caffe2 jobs respectively. In this commit, We have converted all the JOB_BASE_NAME references in .jenkins/pytorch/* files to BUILD_ENVIRONMENT. Then, did the same in .circleci/config.yml. One thing that we needed to be careful was when both BUILD_NAME and JOB_BASE_NAME were present under same declaration in config.yml file (e.g. for "caffe2-" stuffs). To ensure that all "==" checks work as expected, we also had to add "*" in some if conditions in .jenkins/caffe2/build.sh file. Finally, removed "-build", "-test", etc. suffixes from COMPACT_JOB_NAME variable assignment in the bash scriupt files in .jenkins/pytorch folder, e.g., modify COMPACT_JOB_NAME="${BUILD_ENVIRONMENT}-build" to COMPACT_JOB_NAME="${BUILD_ENVIRONMENT}".
@mmh683
Copy link
Contributor Author

mmh683 commented Feb 2, 2019

Incorporated the requested changes into enabled-config.txt file. Now the job names enabled in the text file matches with the corresponding BUILD_ENVIRONMENTs and remaining jenkins builds.

test_custom_script_ops
else
if [[ "${JOB_BASE_NAME}" == *-test1 ]]; then
if [[ "${BUILD_ENVIRONMENT}" == *-test1 ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like dead code to me. @yf225 did there used to be more mac test jobs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to split the test job in half for parallel testing which is what this was for. We can probably keep them around in case we are doing it in the future (which would be a pretty nice CI speed improvement)

fi
else
if [[ "${JOB_BASE_NAME}" == *-test1 ]]; then
if [[ "${BUILD_ENVIRONMENT}" == *-test1 ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks dead as well

$TMP_DIR/ci_scripts/test_libtorch.bat
else
if [[ "${JOB_BASE_NAME}" == *-test1 ]]; then
if [[ "${BUILD_ENVIRONMENT}" == *-test1 ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here as well

Copy link
Member

@kostmo kostmo Feb 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the Windows tests are no longer running at all after this change.

Note that the value of ${JOB_BASE_NAME} used to be pytorch-win-ws2016-cuda9-cudnn7-py3-test1 , but now ${BUILD_ENVIRONMENT} is just pytorch-win-ws2016-cuda9-cudnn7-py3 (without the -test1 suffix), so none of the conditions in the run_tests() function are matched.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed Issue #17101

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmh683 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmh683 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmh683 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmh683 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmh683 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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.

6 participants