Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setup py improvements #38

Merged
merged 3 commits into from
May 13, 2019
Merged

Setup py improvements #38

merged 3 commits into from
May 13, 2019

Conversation

mosra
Copy link
Collaborator

@mosra mosra commented May 9, 2019

Motivation and Context

While #24 said that

do we expect people to be multitasking while installing habitat-sim [...]?

I'm actually one of those crazy multitaskers :) 100% CPU wouldn't be a problem, but when running on all cores, something in the build demands 8 GB of RAM which I usually don't have, so my system comes to a grinding halt. (I still want to investigate what's responsible and if it's possible to tell ninja to not overschedule such jobs.)

While the default is still all the cores, it now allows to override the setting using the setuptools' builtin --parallel option. Together with redirecting it to use a pre-existing CMake build (which was set up manually for #35) dir my command-line looks like this:

python setup.py build_ext --build-temp=build --parallel 3

Apart from this, because I use some system deps, I'm not interested in setup.py implicitly pulling submodules that I am not interested in, so it runs git submodule update only the very first time.

How Has This Been Tested

I'm now able to both have 163 tabs open in my browser and build Habitat :)

Types of changes

  • Docs change / refactoring / dependency upgrade buildsystem update
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation. -- maybe? please tell me where
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

mosra added 2 commits May 9, 2019 22:34
Prevents accidental overwriting of changes (or unnecessary pulling of
more stuff when I explicitly *not* init a subset of the modules).
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label May 9, 2019
@msavva msavva requested review from erikwijmans and msavva May 9, 2019 21:53
setup.py Outdated
# Init & update all submodules if not already (the user might be pinned
# on some particular commit or have working tree changes, don't destroy
# those)
if in_git() and not os.path.exists(".git/modules"):
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of this -- I have resorted to some pretty hacky hacks to get git to not update submodules (adding a random space or newline to a file and then not committing it being my favorite).

However, I also like that currently when people pull a new build, they don't have to worry about updating submodules. Would adding a --no-update-modules flag be sufficient (I am about to submit a PR adding a --cmake-args flag, so I can add it to that)?

CC: @msavva

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see your point. Yes that would work too, I would just need to remember to always add that flag (which is okay) :)

A more general question from a setup.py noob -- do setuptools have something similar to cmake's cache? Like, you enable / set up some flags and properties and they get remembered for the next invocation; so you don't have to specify the whole command-line but can do just ./setup.py and it will pick up the rest from somewhere, which is (like cmake's build dir) not commited to the git repository but private to your working copy? Or am I asking for the impossible?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah... unfortunately setup.py doesn't do that. We could roll our own as pickling out args and reloading if it exists would work.....

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also like the idea. I agree with @erikwijmans 's suggestion to make this into a --no-update-modules flag. It's good to accommodate for the expectation that everything should work through a naive pull-and-build. I'm not aware of a way to make setup.py cache flags etc. :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Made a PR into this PR with a go at caching (it contains more than just that tho).

@mosra
Copy link
Collaborator Author

mosra commented May 10, 2019

Since #40 depends on this (and reverts a part), how should we proceed to merge? Should I remove the .git/submodules part from this one and let it be squashed into #40?

* Argparse
* --cmake-args
* Caching of arguments to build_ext and argparse
@erikwijmans
Copy link
Contributor

Merged #40 into this. This should be good to go too!

Copy link
Collaborator

@msavva msavva left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 Tested build and interactive viewer on MacOS.

@mosra mosra merged commit efd04a2 into master May 13, 2019
@mosra mosra deleted the setup-py-improvements branch May 13, 2019 16:04
Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020
* setup.py: option to don't run git submodule update if modules already exist.

* setup.py: make it possible to *reduce* the amount of jobs.

* Setup.py -- Argprase, Cmake Args, and caching (facebookresearch#40)

* Argparse
* --cmake-args
* Caching of arguments to build_ext and argparse
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants