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

Updates qemu framework to install pip requirements from its own requirements file #14355

Merged
merged 1 commit into from
Mar 7, 2019

Conversation

perdasilva
Copy link
Contributor

@perdasilva perdasilva commented Mar 7, 2019

Description

Removes the installation of tests/requirements.txt. If this file contains requirements for which there are no existing Raspberry Pi wheel files, a build from source will be triggered. This can cause CI timeouts. From now on, the python requirements for qemu tests should to be managed separately.

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

@edisongustavo
Copy link
Contributor

Very interesting findings.

@perdasilva
Copy link
Contributor Author

@marcoabreu could you please merge this? ^^

@marcoabreu
Copy link
Contributor

I don't agree with the approach because it removes transparency and instead makes the project rely on a provided blob (aka the OS image used in the Emulator). Pedro chose this approach specifically to keep up reproducibility.

From what I can tell, we are trying to circumvent a different problem here, which is that windows as well as qemu use the same test requirements.

So I'd either propose to generate a new OS image (preferred solution) or split the requirements.

@perdasilva
Copy link
Contributor Author

perdasilva commented Mar 7, 2019

I agree that this is the preferred solution. But, a few things to keep in mind:

  • this is blocking the windows tests
  • it takes a lifetime to compile scipy in rasp py
  • there is no automation around creating the image
  • the one test we run inside the emulated box does not require scipy
  • we get some transparency by specifying that the mxnet_requirements.txt are what's in the box
  • with the addition of arm instances on aws, it's highly likely we will refactor this approach anyway

@lebeg
Copy link
Contributor

lebeg commented Mar 7, 2019

Good points @perdasilva

@gavinmbell
Copy link

For the sake of maintaining the mental model for future eyeballs to look at. Leaving breadcrumbs in expected places with the right information to show the err of this way makes for a cleaner knowledge trail. Though it is not pragmatic... it is keeping good hygiene for the future.

download

@perdasilva
Copy link
Contributor Author

perdasilva commented Mar 7, 2019

@gavinmbell @marcoabreu I've changed the approach slightly. Is this more agreeable?

re. "From what I can tell, we are trying to circumvent a different problem here, which is that windows as well as qemu use the same test requirements." - actually, some of the tests require numpy and scipy. This issue just wasnt caught before because the test requirements are also installed in the the linux docker environments in install/ubuntu_python.sh (which in this case include scipy).

I guess, previously, the windows ami also pip installed these dependencies, which is why it wasnt caught earlier. Idk.


# CI and Testing

Formally, [runtime_functions.py](https://github.com/apache/incubator-mxnet/blob/master/ci/docker/qemu/runtime_functions.py) would [run](https://github.com/apache/incubator-mxnet/blob/8beea18e3d9835f90b59d3f9de8f9945ac819423/ci/docker/qemu/runtime_functions.py#L81) *pip install -r [mxnet/tests/requirements.txt](https://github.com/apache/incubator-mxnet/blob/master/tests/requirements.txt)*. If the requirements change, there can be an unfortunate side-effect that there are no wheel files for Raspberry Pi for the new requirement. This would trigger a build from source on the emulator, which can take a long time and cause job timeouts. Therefore, we no longer install the `tests/requirements.txt` requirements, but rather rely on [mxnet-requirements.txt](https://github.com/apache/incubator-mxnet/blob/master/ci/qemu/mxnet_requirements.txt) to maintain the requirements for the qemu tests. Should any requirements changes lead to a job time out, it is incumbent on the submitter to update the image to include the requirement and unblock ci.

Choose a reason for hiding this comment

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

Good... So at least there is a pointer for folks to know what does NOT work!
👍

Copy link
Contributor

@marcoabreu marcoabreu left a comment

Choose a reason for hiding this comment

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

Excellent, thanks :)

@marcoabreu
Copy link
Contributor

Nit: You might wanna adapt the title.

@gavinmbell
Copy link

Lets put this to bed Mr nit :-)
im_out_mic_drop

@perdasilva perdasilva changed the title Removes test requirements installation from qemu tests Updates qemu framework to install pip requirements from its own requirements file Mar 7, 2019
@vandanavk
Copy link
Contributor

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

@perdasilva could you have a look at the CI failure?

@marcoabreu marcoabreu added CI pr-awaiting-review PR is waiting for code review labels Mar 7, 2019
@apeforest apeforest merged commit 2b7d57d into apache:master Mar 7, 2019
perdasilva added a commit to perdasilva/incubator-mxnet that referenced this pull request Mar 13, 2019
marcoabreu pushed a commit that referenced this pull request Mar 14, 2019
* Backports Windows CI pipeline fixes from master

* Updates Jenkins file to use python 3 when calling build_windows.py

* Installs qemu pip requirements from qemu requirements file (#14355)

* remove a white space
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
CI pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants