Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Fixes installation nightly test #14144

Merged
merged 1 commit into from
Feb 14, 2019
Merged

Conversation

perdasilva
Copy link
Contributor

@perdasilva perdasilva commented Feb 13, 2019

Description

The nightly job was failing because the installation instructions test was testing v1.3.x instructions against the master branch. I've updated the underlying script to filter out the git calls from the set of operations it extracts from the instruction document.
Targets #13800

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Fixes nightly job by filtering some commands when running the installation guide test.

Comments

This commit was cherry-picked to be merged to branch v1.3.x on PR #14145

@ankkhedia
Copy link
Contributor

@perdasilva Thanks for your contribution!

@mxnet-label-bot add [pr-work-in-progress, test]

@marcoabreu marcoabreu added pr-work-in-progress PR is still work in progress Test labels Feb 13, 2019
@perdasilva perdasilva changed the title [WIP] Fixes installation nightly test Fixes installation nightly test Feb 13, 2019
@perdasilva
Copy link
Contributor Author

@mxnet-label-bot add [pr-awaiting-review]

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Feb 13, 2019
@perdasilva
Copy link
Contributor Author

@mxnet-label-bot remove [pr-work-in-progress]

@marcoabreu marcoabreu removed the pr-work-in-progress PR is still work in progress label Feb 13, 2019
Copy link
Contributor

@ChaiBapchya ChaiBapchya left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Wanted a few changes/clarification. Rest looks good. Also, looks like the RAT license causes CI to fail for sanity job

@@ -250,6 +250,20 @@ function set_instruction_set() {
${sorted_indexes[$end_buildfromsource_command_index]})
}

# given a build commands string, filter any build commands that ought not be executed
# during the test. An example would be git clone'ing the mxnet repository. You want to
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: cloning* and MXNet*

Copy link
Contributor

Choose a reason for hiding this comment

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

Also would like to rephrase the sentence "You want to run the tests against the current commit being tested in Jenkins" - I am unable to understand how does in this example "filter_build_command" play a role?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update the comment - the git clone'ing is to reference the git clone command. But fair comment anyway.

The issues is the build from source commands include cloning the repository, which checks out the master branch. The problem occurs when we are testing build from source commands pinned to v1.3.x against the master branch. This is why the nightly is failing.

The chosen approach to mitigate this was to filter out these git commands (and the cd into the incubator-mxnet directory). This means that the build from source commands will run directly on the version of the repository that is being tested by Jenkins and not against master.

does it make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup. Now it makes sense. Thanks for the clarification.

@perdasilva
Copy link
Contributor Author

@ChaiBapchya I've addressed your comments - please let me know if the issue is clearer now and the commend over the function more helpful

Copy link
Contributor

@ChaiBapchya ChaiBapchya left a comment

Choose a reason for hiding this comment

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

LGTM w/o spelling mistakes. Thanks for the PR!

# $ git clone --recursive https://github.com/apache/incubator-mxnet
# $ cd incubator-mxnet
# if these commands get executed in the jenkins job, we will be testing the build from source instructions
# against the master branch and not against the version of the repository that Jenkins checksout for testing.
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: checks out

# This presents a particularly big problem for the version branches and their nightly builds. Because,
# we would, in effect, be testing the build from source instructions for one version of MXNet against
# the master branch.
# in this function we target the commands cited in the example above, but leave it open for expantion
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: "In" "explanation"

@marcoabreu
Copy link
Contributor

Please address the comments in a separate PR. Merging to unblock pipeline.

@marcoabreu marcoabreu merged commit 074a859 into apache:master Feb 14, 2019
@perdasilva perdasilva mentioned this pull request Feb 14, 2019
5 tasks
@perdasilva perdasilva deleted the testfix branch February 14, 2019 19:32
stephenrawls pushed a commit to stephenrawls/incubator-mxnet that referenced this pull request Feb 16, 2019
jessr92 pushed a commit to jessr92/incubator-mxnet that referenced this pull request Feb 19, 2019
drivanov pushed a commit to drivanov/incubator-mxnet that referenced this pull request Mar 4, 2019
vdantu pushed a commit to vdantu/incubator-mxnet that referenced this pull request Mar 31, 2019
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review Test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants