Improve developer documentation and experience (#184)

* De-duplicate and reorganize developer documentation

* Improve makefile error when submodules missing

* Tweak pylint settings

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.

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

docs/internals.rst

@@ -12,6 +12,7 @@ going on "behind the scenes".
     :caption: Contents:
 
     internals/ffi
+    internals/development
     internals/testing
     internals/documentation
 

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 <R-compat>`, 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 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
+------
+
+* Python dependencies:
+  * `NumPy <https://numpy.org/>`_
+
+* We autoformat code with `black <https://black.readthedocs.io/en/stable/>`_.
+
+Julia
+-----
+
+* Julia dependencies:
+  * `LazyArtifacts <https://docs.julialang.org/en/v1/stdlib/LazyArtifacts/>`_
+
+* Julia code is formatted using `JuliaFormatter <https://github.com/domluna/JuliaFormatter.jl>`_.
+
+R
+-
+
+* R dependencies:
+  * `R6 <https://cran.r-project.org/web/packages/R6/index.html>`_
+
+Rust
+----
+
+* Rust development is based on :command:`cargo`, which should handle dependencies and formatting.

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
 ______
 

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
 ----------------------
 

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

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()

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:

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"""