From 9fecdb3e6f7f0ccea5afdd433a4350fba07e3b10 Mon Sep 17 00:00:00 2001 From: Brian Ward Date: Mon, 6 Nov 2023 15:12:43 -0500 Subject: [PATCH] Improve developer documentation and experience (#184) * De-duplicate and reorganize developer documentation * Improve makefile error when submodules missing * Tweak pylint settings --- CONTRIBUTING.md | 62 +++------------------------------- Makefile | 38 +++++++++++++++++++-- docs/internals.rst | 1 + docs/internals/development.rst | 55 ++++++++++++++++++++++++++++++ docs/internals/testing.rst | 3 -- docs/languages/c-api.rst | 2 ++ python/.pylintrc | 28 +++++---------- python/bridgestan/download.py | 12 +++---- python/bridgestan/model.py | 11 +++--- python/test/test_stanmodel.py | 2 +- 10 files changed, 118 insertions(+), 96 deletions(-) create mode 100644 docs/internals/development.rst diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e2223622..a0961344 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -2,7 +2,9 @@ We welcome contributions to the project in any form, including bug reports, bug fixes, new features, improved documentation, or improved test coverage. -Developer-specific documentation is available at https://roualdes.github.io/bridgestan/latest/internals.html +**Developer-specific documentation is available [as part of our online documentation](https://roualdes.github.io/bridgestan/latest/internals.html)**. Information on building BridgeStan, running tests, and developing with the project is available there. + +This document houses additional information about licensing and procedures for contributing to the project. ## Licensing @@ -16,7 +18,7 @@ We follow the practices laid out in the [GitHub Terms of Service](https://docs.g * Developers (or their assignees) retain copyright for their doc and code contributions. * Developers agree to release their contributed code and documentation under the repository licenses (see above). -## Developers +## Development Process We follow standard open source and GitHub practices: @@ -28,62 +30,6 @@ We follow standard open source and GitHub practices: * We propose updates through [GitHub Pull Requests](https://github.com/roualdes/bridgestan/pulls) so that we can do code review. We do not push directly to the main branch. - -## Builds - -We use [Gnu make](https://www.gnu.org/software/make/) for builds. If you have previously used [CmdStan](https://mc-stan.org/users/interfaces/cmdstan), then you will already have Gnu make and a sufficient C++ compiler. - - -## Language specific practices - -### C and C++ development - -* We require the following dependency for C++ development: - -* We use the C++1y standard for compilation (`-std=c++1y` in both clang and gcc). This is partway between C++11 and C++14, and is what Stan requires. - -* We try to write standards-compliant code that does not depend on features of specific platforms (except where needed for compatibility). Specifically, we do not use OS-dependent or compiler-dependent C++. Our C++ code does not depend on the `R.h` or `Python.h` headers, for example. On the other hand, adding new signatures to work with a specific language's style of foreign function interface is permitted (an example can be found in the R interface, which requires a particular pointer-based style). - -* We try to follow the [Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html), but (a) we allow C++ exceptions, and (b) we allow reference arguments. - -* We will use [GoogleTest](https://google.github.io/googletest/) for C++ unit testing. - -* We recommend using [Clang format](https://clang.llvm.org/docs/ClangFormat.html) with our config file [`.clang-format`](https://github.com/roualdes/bridgestan/blob/main/.clang-format). - - -### Python development - -* Python development relies on the external dependencies: - * [NumPy](https://numpy.org/) - -* We autoformat code with [black](https://black.readthedocs.io/en/stable/). - -* We recommend but do not require checking the code style and semantics with [PyLint](https://www.pylint.org) using [BridgeStan .pylintrc](https://github.com/roualdes/bridgestan/blob/main/.pylintrc). - -### R development - -* R dependencies beyond those included in this repo: - * [R6 package](https://cran.r-project.org/web/packages/R6/index.html) for reference classes - -### Julia development - -* Julia dependencies beyond those included in this repo: - * [Test](https://docs.julialang.org/en/v1/stdlib/Test/) - * [LinearAlgebra](https://docs.julialang.org/en/v1/stdlib/LinearAlgebra/) - -* Julia code is formatted using[JuliaFormatter](https://github.com/domluna/JuliaFormatter.jl). - -* Julia's multi-threading capabilities allow different processors/threads to -make simultaneous calls to the BridgeStan API. Such capabilities require the -target Stan program to be compiled with `STAN_THREADS=true`, see the function -[compile_model](https://roualdes.github.io/bridgestan/latest/languages/julia.html#BridgeStan.compile_model) -for more details. The Julia interface tests this feature and thus requires -`STAN_THREADS=true` for the tests to run successfully. - -### Rust development - -* Rust development is based on `cargo`, which should handle dependencies, testing, and formatting. - ## Proposing a new interface language If you would like to extend BridgeStan to a language besides Python, R, or Julia, please open a [GitHub Issue](https://github.com/roualdes/bridgestan/issues) to discuss your proposal. diff --git a/Makefile b/Makefile index 4b520e7a..8257744d 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,9 @@ ## include paths BS_ROOT ?= . + +# user config +-include $(BS_ROOT)/make/local + SRC ?= $(BS_ROOT)/src/ STAN ?= $(BS_ROOT)/stan/ STANC ?= $(BS_ROOT)/bin/stanc$(EXE) @@ -10,9 +14,8 @@ RAPIDJSON ?= $(STAN)lib/rapidjson_1.1.0/ INC_FIRST ?= -I $(STAN)src -I $(RAPIDJSON) ## makefiles needed for math library --include $(BS_ROOT)/make/local --include $(MATH)make/compiler_flags --include $(MATH)make/libraries +include $(MATH)make/compiler_flags +include $(MATH)make/libraries ## Set -fPIC globally since we're always building a shared library CXXFLAGS += -fPIC @@ -96,6 +99,19 @@ stan-update-remote: compile_info: @echo '$(LINK.cpp) $(BRIDGE_O) $(LDLIBS) $(SUNDIALS_TARGETS) $(MPI_TARGETS) $(TBB_TARGETS)' +# set to false if you want to fail on formatting errors +# e.g., in continuous integration +BS_FORMAT_IGNOREABLE ?= true + +.PHONY: format +format: + clang-format -i src/*.cpp src/*.hpp src/*.h || $(BS_FORMAT_IGNOREABLE) + isort python || $(BS_FORMAT_IGNOREABLE) + black python || $(BS_FORMAT_IGNOREABLE) + julia --project=julia -e 'using JuliaFormatter; format("julia/")' || $(BS_FORMAT_IGNOREABLE) +# R seems to have no good formatter that doesn't choke on doc comments +# Rscript -e 'formatR::tidy_dir("R/", recursive=TRUE)' || $(BS_FORMAT_IGNOREABLE) + ## print value of makefile variable (e.g., make print-TBB_TARGETS) .PHONY: print-% print-% : ; @echo $* = $($*) ; @@ -141,3 +157,19 @@ $(STANC): curl -L https://github.com/stan-dev/stanc3/releases/download/$(STANC3_VERSION)/$(OS_TAG)$(ARCH_TAG)-stanc -o $(STANC) --retry $(STANC_DL_RETRY) --retry-delay $(STANC_DL_DELAY) chmod +x $(STANC) endif + +## +# This is only run if the `include` statements earlier fail to find a file. +# We assume that means the submodule is missing +## +$(MATH)make/% : + @echo 'ERROR: Missing Stan submodules.' + @echo 'We tried to find the Stan Math submodule at:' + @echo ' $(MATH)' + @echo '' + @echo 'The most likely source of the problem is BridgeStan was cloned without' + @echo 'the --recursive flag. To fix this, run the following command:' + @echo ' git submodule update --init --recursive' + @echo '' + @echo 'And try building again' + @exit 1 diff --git a/docs/internals.rst b/docs/internals.rst index 93d95a77..09771a53 100644 --- a/docs/internals.rst +++ b/docs/internals.rst @@ -12,6 +12,7 @@ going on "behind the scenes". :caption: Contents: internals/ffi + internals/development internals/testing internals/documentation diff --git a/docs/internals/development.rst b/docs/internals/development.rst new file mode 100644 index 00000000..985e6b8a --- /dev/null +++ b/docs/internals/development.rst @@ -0,0 +1,55 @@ +Developer Information +===================== + +The following information is intended for developers who wish to modify the +code. + +The pages on :doc:`testing` and :doc:`documentation` are also relevant. + +Building BridgeStan +------------------- + +Developers should follow the instructions in :doc:`../getting-started` to ensure +they have the minimal C++ build tools required for working with BridgeStan. + +C++ +--- + +* We use the C++1y standard for compilation (``-std=c++1y`` in both clang and gcc). This is partway between C++11 and C++14, and is what Stan requires. + +* We try to write standards-compliant code that does not depend on features of specific platforms (except where needed for compatibility). + Specifically, we do not use OS-dependent or compiler-dependent C++. + Our C++ code does not depend on the ``R.h`` or ``Python.h`` headers, for example. + On the other hand, adding new signatures to work with a specific language's style of foreign function interface is permitted + (an example can be found in the :ref:`R compatibility functions `, which requires a particular pointer-based style). + +* We try to follow the `Google C++ Style Guide `_, but (a) we allow C++ exceptions, and (b) we allow reference arguments. + +* We recommend using `Clang format `_ with our config file `.clang-format `_. + +Python +------ + +* Python dependencies: + * `NumPy `_ + +* We autoformat code with `black `_. + +Julia +----- + +* Julia dependencies: + * `LazyArtifacts `_ + +* Julia code is formatted using `JuliaFormatter `_. + +R +- + +* R dependencies: + * `R6 `_ + +Rust +---- + +* Rust development is based on :command:`cargo`, which should handle dependencies and formatting. diff --git a/docs/internals/testing.rst b/docs/internals/testing.rst index 71a6f993..87786b8d 100644 --- a/docs/internals/testing.rst +++ b/docs/internals/testing.rst @@ -16,9 +16,6 @@ Note: The additional functionality provided by but in order to facilitate the same built models being used in all tests we use it regardless of interface. -Tooling -------- - Python ______ diff --git a/docs/languages/c-api.rst b/docs/languages/c-api.rst index ea437333..cf7d8b8e 100644 --- a/docs/languages/c-api.rst +++ b/docs/languages/c-api.rst @@ -42,6 +42,8 @@ These functions are implemented in C++, see :doc:`../internals` for more details :project: bridgestan :sections: func typedef var +.. _R-compat: + R-compatible functions ---------------------- diff --git a/python/.pylintrc b/python/.pylintrc index d612cb02..f090ba26 100644 --- a/python/.pylintrc +++ b/python/.pylintrc @@ -3,11 +3,11 @@ # A comma-separated list of package or module names from where C extensions may # be loaded. Extensions are loading into the active Python interpreter and may # run arbitrary code. -extension-pkg-whitelist=ujson +extension-pkg-whitelist= # Add files or directories to the blacklist. They should be base names, not # paths. -ignore=docsrc/ +ignore= # Add files or directories matching the regex patterns to the blacklist. The # regex matches against base names, not paths. @@ -60,22 +60,10 @@ confidence= # --enable=similarities". If you want to run only the classes checker, but have # no Warning level messages displayed, use "--disable=all --enable=classes # --disable=W". -disable=consider-using-in, - cyclic-import, - fixme, - import-outside-toplevel, - no-else-break, - no-else-return, - protected-access, - redefined-builtin, - redundant-unittest-assert, - too-many-branches, - too-many-locals, - unsubscriptable-object, - consider-using-with, - consider-using-dict-items, - unspecified-encoding, - consider-using-f-string, +disable=missing-module-docstring, + global-statement, + bare-except, + too-many-instance-attributes # Enable the message, report, category or checker with the given id(s). You can # either give multiple identifier separated by comma (,) or put this option @@ -355,7 +343,9 @@ good-names=i, xs, y, Run, - _ + _, + lp, + rc # Include a hint for the correct naming format with invalid-name. include-naming-hint=no diff --git a/python/bridgestan/download.py b/python/bridgestan/download.py index 2fe8bb65..1bf0a1cd 100644 --- a/python/bridgestan/download.py +++ b/python/bridgestan/download.py @@ -36,14 +36,12 @@ def get_bridgestan_src(): print(err_text) if i == RETRIES: raise ValueError(err_text) from e - else: - print("Retrying ({i+1}/{RETRIES})...") - sleep(1) + + print("Retrying ({i+1}/{RETRIES})...") + sleep(1) try: - tar = tarfile.open(file_tmp) - tar.extractall(path=HOME_BRIDGESTAN) + with tarfile.open(file_tmp) as tar: + tar.extractall(path=HOME_BRIDGESTAN) except Exception as e: raise ValueError(f"Failed to unpack {file_tmp} during installation") from e - finally: - tar.close() diff --git a/python/bridgestan/model.py b/python/bridgestan/model.py index 042f9d3d..3bc57d27 100644 --- a/python/bridgestan/model.py +++ b/python/bridgestan/model.py @@ -66,15 +66,16 @@ def __init__( validate_readable(model_lib) if model_data is not None and model_data.endswith(".json"): validate_readable(model_data) - with open(model_data, "r") as f: - model_data = f.read() + with open(model_data, "r", encoding="utf-8") as file: + model_data = file.read() windows_dll_path_setup() self.lib_path = str(Path(model_lib).absolute().resolve()) if self.lib_path in dllist(): warnings.warn( f"Loading a shared object {self.lib_path} that has already been loaded.\n" - "If the file has changed since the last time it was loaded, this load may not update the library!" + "If the file has changed since the last time it was loaded, this load may " + "not update the library!" ) self.stanlib = ctypes.CDLL(self.lib_path) @@ -681,8 +682,8 @@ def _handle_error(self, err: ctypes.c_char_p, method: str) -> Exception: string = ctypes.string_at(err).decode("utf-8") self._free_error(err) return RuntimeError(string) - else: - return RuntimeError(f"Unknown error in {method}. ") + + return RuntimeError(f"Unknown error in {method}. ") class StanRNG: diff --git a/python/test/test_stanmodel.py b/python/test/test_stanmodel.py index 9af2481a..5aa479e2 100644 --- a/python/test/test_stanmodel.py +++ b/python/test/test_stanmodel.py @@ -711,12 +711,12 @@ def test_reload_warning(): data = str(STAN_FOLDER / "fr_gaussian" / "fr_gaussian.data.json") model = bs.StanModel(str(lib), data) - relative_lib = lib.relative_to(STAN_FOLDER.parent) assert not relative_lib.is_absolute() with pytest.warns(UserWarning, match="may not update the library"): model2 = bs.StanModel(str(relative_lib), data) + @pytest.fixture(scope="module") def recompile_simple(): """Recompile simple_model with autodiff hessian enable, then clean-up/restore it after test"""