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

Add setuptools config, set up versioneer #597

Merged
merged 24 commits into from
Feb 3, 2022
Merged

Add setuptools config, set up versioneer #597

merged 24 commits into from
Feb 3, 2022

Conversation

mcwitt
Copy link
Collaborator

@mcwitt mcwitt commented Feb 2, 2022

  • Adds project setup.py, adapted from https://github.com/pypa/sampleproject/blob/main/setup.py
  • Adds configuration for versioneer to handle versioning based on git tags (set up following instructions here)
  • Builds C++ lib with setuptools. Notes:
    • current method using make build still works
    • default CUDA_ARCH is now defined in CMakeLists.txt (as opposed to Makefile)
  • Changes the default CUDA_ARCH to 75 (this is what we're using in ansible builds)
  • Updates setup instructions

Versioning

Currently we get

>>> import timemachine as tm
>>> tm.__version__
'0+untagged.642.gda016b2'

After tagging, e.g.

git tag -a 0.1.0

we'll get

>>> tm.__version__
'0.1.0'

After the next (untagged) commit, it'll look like

>>> tm.__version__
'0.1.0+1.g7ac99fd'

setup.py Show resolved Hide resolved
These can be installed via `pip install .[dev]`
black
pytest
pytest-cov
pre-commit
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dev and testing dependencies can be installed via pip install .[dev] or pip install .[test] respectively.

setup.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
@mcwitt
Copy link
Collaborator Author

mcwitt commented Feb 2, 2022

Force-pushed to remove some local configuration I had accidentally committed.

@@ -10,7 +10,7 @@ repos:
rev: 21.10b0
hooks:
- id: black
args: [--line-length=120]
exclude: '^versioneer.py$|^timemachine/_version.py'
Copy link
Collaborator Author

@mcwitt mcwitt Feb 2, 2022

Choose a reason for hiding this comment

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

Unfortunately these exclusions need to be duplicated here from pyproject.toml (unless we want to use force-exclude, which would prevent any invocation of black from formatting these files; see psf/black#438)

@@ -29,7 +29,7 @@ if(NOT EXISTS ${EIGEN_SRC_DIR})
execute_process(COMMAND git clone --branch 3.3.9 https://gitlab.com/libeigen/eigen.git ${EIGEN_SRC_DIR})
endif()

add_subdirectory(${CMAKE_CURRENT_BINARY_DIR}/${PYBIND_SRC_DIR})
add_subdirectory(${CMAKE_CURRENT_BINARY_DIR}/${PYBIND_SRC_DIR} ${CMAKE_CURRENT_BINARY_DIR}/${PYBIND_SRC_DIR})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without this I get

CMake Error at CMakeLists.txt:32 (add_subdirectory):
  add_subdirectory not given a binary directory but the given source
  directory
  ...
  When specifying an out-of-tree source a binary directory must be explicitly
  specified.

@@ -65,7 +65,13 @@ include_directories(${CMAKE_CURRENT_BINARY_DIR}/eigen)
include_directories(${CMAKE_CURRENT_BINARY_DIR}/${CUB_SRC_DIR})

set_property(TARGET ${LIBRARY_NAME} PROPERTY CUDA_STANDARD 14)

if (NOT CUDA_ARCH)
set(CUDA_ARCH "75")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is to allow pip install . to succeed without needing to first set e.g. CMAKE_ARGS="-DCUDA_ARCH=75"

@mcwitt mcwitt marked this pull request as draft February 2, 2022 16:19
@mcwitt
Copy link
Collaborator Author

mcwitt commented Feb 2, 2022

Moving this to draft status as there are still a few issues to work out with the setuptools build.

@@ -6,8 +6,6 @@ PYTEST_CI_ARGS := --cov=. --cov-report=term-missing

NPROCS = `nproc`

CUDA_ARCH := "70"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The default CUDA_ARCH setting was moved to CMakeLists.txt. This will allow pip install -e . to succeed without needing to set the CMake flags.

@mcwitt mcwitt marked this pull request as ready for review February 2, 2022 19:15
@mcwitt mcwitt requested review from maxentile and badisa and removed request for badisa February 2, 2022 19:44
setup.py Show resolved Hide resolved
versioneer.py Show resolved Hide resolved
Copy link
Collaborator

@maxentile maxentile left a comment

Choose a reason for hiding this comment

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

These seem like great changes. Tested out the updated instructions for installing in develop mode and it all worked as expected. (Also quite nice to be able to access timemachine.__version__ from within a script!)

@mcwitt mcwitt merged commit e99b61b into master Feb 3, 2022
@mcwitt mcwitt deleted the project-config branch February 3, 2022 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants