Skip to content

Commit

Permalink
C++ improvements (#63)
Browse files Browse the repository at this point in the history
* Refactor cpp structure

* Start to add tests

* Try to clean up my mess a bit.  Also add PR template

* Clean up documentation and got tests running

* Uncomment the other tests but they failing right now

* Fix java and python cpp src paths

* Improve formatting strategy

* Run C++ formatter

* Add .clang-format to root for CI

* Update clang-format action container

* Fix find command

* Start to replace Catch2 with doctest. Dummy test only

* Remove commented test code until namespacing is implemented

* Add some actual doctest tests

* Use doctest and add combination var checks

* Add c++ tests to github workflows

* Fix rebase error

* Update javadoc

* Undo docs changes for cleaner PR

* Add EOF EOL

* Revert cpp spacing to 80 for cleaner PR

* Fix TypedIndex rebase

* Add c++ tests to github actions

* Add build dir for cmake

* Fix windows tests
  • Loading branch information
markkohdev authored Aug 20, 2024
1 parent fd2feda commit a4902b8
Show file tree
Hide file tree
Showing 42 changed files with 344 additions and 112 deletions.
1 change: 1 addition & 0 deletions .clang-format
36 changes: 36 additions & 0 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
## Description
<!-- Describe the changes introduced by this pull request and why they are necessary. -->

## Related Issues
<!-- Reference any related GitHub issues that are addressed by this pull request. -->

## Changes Made
<!-- Provide a brief overview of the changes made in this pull request. -->

### C++
<!-- Describe changes made to the C++ code, including any new features, improvements, or bug fixes. -->

### Python
<!-- Describe changes made to the Python code, including any new features, improvements, or bug fixes. -->

### Java
<!-- Describe changes made to the Java code, including any new features, improvements, or bug fixes. -->

## Testing
<!-- Outline the testing strategy employed to validate these changes. Include any relevant test cases or scenarios. -->

## Checklist
<!--
Replace [ ] with [x] to check off items.
For items that are not applicable, simply remove the checkbox.
-->

- [ ] My code follows the code style of this project.
- [ ] I have added and/or updated appropriate documentation (if applicable).
- [ ] All new and existing tests pass locally with these changes.
- [ ] I have run static code analysis (if available) and resolved any issues.
- [ ] I have considered backward compatibility (if applicable).
- [ ] I have confirmed that this PR does not introduce any security vulnerabilities.

## Additional Comments
<!-- Add any additional comments or notes that might be helpful for reviewers or contributors. -->
34 changes: 32 additions & 2 deletions .github/workflows/all.yml
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,38 @@ jobs:
- name: Check C++ Formatting
uses: jidicula/[email protected]
with:
clang-format-version: 14
fallback-style: LLVM
clang-format-version: 16

run-cpp-tests:
runs-on: ${{ matrix.os }}
continue-on-error: true
defaults:
run:
working-directory: cpp
strategy:
matrix:
# TODO: Switch back to macos-latest once https://github.com/actions/python-versions/pull/114 is fixed
os:
- 'ubuntu-latest'
- windows-latest
- macos-12
name: Test C++ on ${{ matrix.os }}
steps:
- uses: actions/checkout@v3
with:
submodules: recursive
- name: Install CMake (Windows)
if: matrix.os == 'windows-latest'
run: |
choco install cmake --installargs 'ADD_CMAKE_TO_PATH=System'
- name: Install CMake (MacOS)
if: matrix.os == 'macos-12'
run: brew install cmake
- name: Install CMake (Ubuntu)
if: matrix.os == 'ubuntu-latest'
run: sudo apt-get install -y cmake
- name: Run tests
run: make test

run-java-tests:
continue-on-error: true
Expand Down
19 changes: 18 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
*.egg-info/
build/
*/build/*
!build/.gitkeep
dist/
tmp/
__pycache__/
Expand All @@ -18,3 +19,19 @@ java/classpath.txt
java/linux-build/include/*
python/voyager-headers
.asv/

# Cmake
CMakeLists.txt.user
CMakeCache.txt
CMakeFiles
CMakeScripts
Testing
Makefile
!cpp/Makefile
cmake_install.cmake
install_manifest.txt
compile_commands.json
CTestTestfile.cmake
_deps
DartConfiguration.tcl
VoyagerTests
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[submodule "cpp/include/doctest"]
path = cpp/include/doctest
url = [email protected]:doctest/doctest.git
176 changes: 86 additions & 90 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -1,91 +1,81 @@
# How to Contribute

We'd love to get patches from you!

## Getting Started
#### Workflow
We follow the [GitHub Flow Workflow](https://guides.github.com/introduction/flow/):

### Prerequisites
1. Fork the project
1. Check out the `master` branch
1. Create a feature branch
1. Write code and tests for your change
1. From your branch, make a pull request against `https://github.com/spotify/voyager`
1. Work with repo maintainers to get your change reviewed
1. Wait for your change to be pulled into `https://github.com/spotify/voyager/master`
1. Delete your feature branch

## Getting Started
### Prerequisites
To compile Voyager from scratch, the following packages will need to be installed:

- [Python 3.7](https://www.python.org/downloads/) or higher.
- A C++ compiler, e.g. `gcc`, `clang`, etc.

### Building Voyager
#### Building Python
There are some nuances to building the Voyager python code. Please read on for more information.

For basic building, you should be able to simply run the following commands:
```shell
git clone [email protected]:spotify/voyager.git
cd voyager
pip3 install -r python/dev-requirements.txt
pip3 install .
cd python
pip install -r python/dev-requirements.txt
pip install .
```

To compile a debug build of `voyager` that allows using a debugger (like gdb or lldb), use the following command to build the package locally and install a symbolic link for debugging:
```shell
cd python
DEBUG=1 python3 setup.py build develop
DEBUG=1 python setup.py build develop
```

Then, you can `import voyager` from Python (or run the tests with `tox`) to test out your local changes.

> If you're on macOS or Linux, you can try to compile a debug build _faster_ by using [Ccache](https://ccache.dev/):
> ## macOS
> ```shell
> brew install ccache
> rm -rf build && CC="ccache clang" CXX="ccache clang++" DEBUG=1 python3 -j8 -m pip install -e .
> rm -rf build && CC="ccache clang" CXX="ccache clang++" DEBUG=1 python -j8 -m pip install -e .
> ```
> ## Linux
> e.g.
> ```shell
> sudo yum install ccache # or apt, if on a Debian
>
> # If using GCC:
> rm -rf build && CC="ccache gcc" CXX="scripts/ccache_g++" DEBUG=1 python3 setup.py build -j8 develop
> rm -rf build && CC="ccache gcc" CXX="scripts/ccache_g++" DEBUG=1 python setup.py build -j8 develop
>
> # ...or if using Clang:
> rm -rf build && CC="ccache clang" CXX="scripts/ccache_clang++" DEBUG=1 python3 setup.py build -j8 develop
> rm -rf build && CC="ccache clang" CXX="scripts/ccache_clang++" DEBUG=1 python setup.py build -j8 develop
> ```
### Updating Documentation
If you notice that the documentation is out of date, feel free to run these commands in order to update the docs and make a PR with the changes.
#### Python
While `voyager` is mostly C++ code, it ships with `.pyi` files to allow for type hints in text editors and via MyPy. To update the Python type hint files, use the following commands:
```shell
cd python
python3 -m scripts.generate_type_stubs_and_docs
# Documentation will be dumped into ../docs/python/
```
#### Java
To update the javadocs for the java bindings, you can simply run:
#### Building Java
To build the Java library with `maven`, use the following commands:
```shell
cd java
mvn package
```
this will update the java documentation located in [docs/java/](https://github.com/spotify/voyager/tree/main/docs/java).
## Workflow
We follow the [GitHub Flow Workflow](https://guides.github.com/introduction/flow/):
1. Fork the project
1. Check out the `master` branch
1. Create a feature branch
1. Write code and tests for your change
1. From your branch, make a pull request against `https://github.com/spotify/voyager`
1. Work with repo maintainers to get your change reviewed
1. Wait for your change to be pulled into `https://github.com/spotify/voyager/master`
1. Delete your feature branch
#### Building C++
To build the C++ library with `cmake`, use the following commands:
```shell
cd cpp
git submodule update --init --recursive
make build
```

## Testing
### Python Tests
We use `tox` for testing - running tests from end-to-end should be as simple as:

```
cd python
pip3 install tox
tox
```
Expand All @@ -110,10 +100,26 @@ asv continuous --sort name --no-only-changed HEAD main
Please note that `airspeed-velocity` can only run benchmarks against a git commit, so if
you have uncommited code that you want to run benchmarks for you need to commit it first.

## Style
### Java Tests
We provide java test execution as a maven test step. Thus you can run the tests with:

```shell
cd java
mvn verify
```

### C++ Tests
To run the C++ tests, use the following commands:
```shell
cd cpp
git submodule update --init --recursive
make test
```

## Style
Use [`clang-format`](https://clang.llvm.org/docs/ClangFormat.html) for C++ code, and `black` with defaults for Python code.

### Python
In order to check and run formatting within the python module, you can use tox to facilitate this.
```bash
cd python
Expand All @@ -123,8 +129,43 @@ tox -e check-formatting
tox -e format
```

## Issues
### C++
If you are working on any C++ code throughout the repo, ensure you have `clang-format` (version 16) installed, and then use clang-format to handle C++ formatting:
```bash
cd cpp
cmake .
# Check formatting only (don't change files)
make check-formatting
# Run formatter
make format
```

### Updating Documentation
We also welcome improvements to the project documentation or to the existing
docs. Please file an [issue](https://github.com/spotify/voyager/issues/new).

If you notice that the generated API documentation is out of date, feel free to run these commands in order to update the docs and make a PR with the changes.

#### Python
While `voyager` is mostly C++ code, it ships with `.pyi` files to allow for type hints in text editors and via MyPy. To update the Python type hint files, use the following commands:

```shell
cd python
python3 -m scripts.generate_type_stubs_and_docs
# Documentation will be dumped into ../docs/python/
```

#### Java
To update the javadocs for the java bindings, you can simply run:

```shell
cd java
mvn package
```

This will update the java documentation located in [docs/java/](https://github.com/spotify/voyager/tree/main/docs/java).

## Issues
When creating an issue please try to ahere to the following format:

One line summary of the issue (less than 72 characters)
Expand All @@ -141,47 +182,7 @@ When creating an issue please try to ahere to the following format:

List all relevant steps to reproduce the observed behaviour.

## Pull Requests
Files should be exempt of trailing spaces.
We adhere to a specific format for commit messages. Please write your commit
messages along these guidelines. Please keep the line width no greater than 80
columns (You can use `fmt -n -p -w 80` to accomplish this).
One line description of your change (less than 72 characters)
Problem
Explain the context and why you're making that change. What is the problem
you're trying to solve? In some cases there is not a problem and this can be
thought of being the motivation for your change.
Solution
Describe the modifications you've done.
Result
What will change as a result of your pull request? Note that sometimes this
section is unnecessary because it is self-explanatory based on the solution.
Some important notes regarding the summary line:
* Describe what was done; not the result
* Use the active voice
* Use the present tense
* Capitalize properly
* Do not end in a period — this is a title/subject
* Prefix the subject with its scope
## Documentation
We also welcome improvements to the project documentation or to the existing
docs. Please file an [issue](https://github.com/spotify/voyager/issues/new).
## First Contributions
If you are a first time contributor to `voyager`, familiarize yourself with the:
* [Code of Conduct](CODE_OF_CONDUCT.md)
* [GitHub Flow Workflow](https://guides.github.com/introduction/flow/)
Expand All @@ -192,20 +193,15 @@ When you're ready, navigate to [issues](https://github.com/spotify/voyager/issue
There is a lot to learn when making your first contribution. As you gain experience, you will be able to make contributions faster. You can submit an issue using the [question](https://github.com/spotify/voyager/labels/question) label if you encounter challenges.

# License
By contributing your code, you agree to license your contribution under the
terms of the [LICENSE](https://github.com/spotify/voyager/blob/master/LICENSE).

# Code of Conduct
Read our [Code of Conduct](CODE_OF_CONDUCT.md) for the project.

# Troubleshooting
## Building the project
### `ModuleNotFoundError: No module named 'pybind11'`
Try updating your version of `pip`:
```shell
pip install --upgrade pip
Expand Down
8 changes: 8 additions & 0 deletions cpp/.clang-format
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
BasedOnStyle: LLVM
IndentWidth: 2
InsertNewlineAtEOF: true
---
Language: Cpp
# Use 120 columns since we have big screens now
ColumnLimit: 80
Loading

0 comments on commit a4902b8

Please sign in to comment.