-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Improve dev_menu usability, local build and virtualenv #13529
Conversation
@mxnet-label-bot add [pr-awaiting-review] |
The menu is nice. |
@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? |
4af7b1c
to
6265ee4
Compare
ci/requirements_mxnet_virtualenv.txt
Outdated
@@ -0,0 +1,6 @@ | |||
opencv-python |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
…l builds for easy testing
Co-Authored-By: larroy <[email protected]>
@marcoabreu I changed what you requested, can we merge please if you don't have any more concerns? |
Description
Adds a convenient
./dev_menu.py build
command which does a local build with the settings defined in cmake_options.yamlAdds 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.