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

Improve dev_menu usability, local build and virtualenv #13529

Merged
merged 4 commits into from
Dec 13, 2018

Conversation

larroy
Copy link
Contributor

@larroy larroy commented Dec 4, 2018

Description

Adds a convenient ./dev_menu.py build command which does a local build with the settings defined in cmake_options.yaml

Adds capability to create a local environment where built mxnet is installed for testing & other tasks.

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:https://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

@larroy
Copy link
Contributor Author

larroy commented Dec 4, 2018

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

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Dec 4, 2018
@aaronmarkham
Copy link
Contributor

The menu is nice.
Is defaulting to CUDA=ON the preferred way? I thought OFF was the default with make right now...
I was a little confused with the mixing of cmake_options.yaml and cmake_options.yml. Can we have just one reference?

dev_menu.py Outdated Show resolved Hide resolved
@larroy
Copy link
Contributor Author

larroy commented Dec 6, 2018

@aaronmarkham thanks for the feedback. We can default to cuda OFF. Where are the duplicated references from yml yaml? this must have been a typing issue. Any additional suggestions?

@@ -0,0 +1,6 @@
opencv-python
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these requirements duplicated? I feel like they are pretty generic and should be in another place. could you check if there's an existing requirements.txt that we can use? otherwise, we have too many requirements files which makes us prone to miss one

Copy link
Contributor Author

@larroy larroy Dec 11, 2018

Choose a reason for hiding this comment

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

There are no suitable file with such requirements other than ci/docker/install/ubuntu_tutorials.sh let me know if you have suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is different. The requirements file I provided is for running mxnet on the console, opencv is not needed for the test, is needed for examples and other things. Better keep them separate.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do need opencv for the tests afaik

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are working right? Why should I change the requirements for unit tests on this PR? it could introduce errors in CI like last time. I think that's better done in a separate PR. Is your concern addressed?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't use that linked requirements.txt in our CI. Instead, we install OpenCV and all that as part of our docker setup.

If it would introduce errors, we would see it as part of the PR validation. Please, add your requirements to the linked requirements file and delete the one you created since they are redundant. The linked file is exactly for the purpose of providing an environment to run tests, which is also the purpose of your new requirements file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is going to have implications on other places, in qemu we are installing those with:

81: check_call(['sudo', 'pip3', 'install', '-r', 'mxnet/tests/requirements.txt'])

Any additional requirement can take a lot of time to compile in QEMU, I don't want to add requirements to the test files that are not needed.

I'm against the change that you propose because this reason.

@larroy
Copy link
Contributor Author

larroy commented Dec 13, 2018

@marcoabreu I changed what you requested, can we merge please if you don't have any more concerns?

@marcoabreu marcoabreu merged commit a1cc2ba into apache:master Dec 13, 2018
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants