All reviews of published articles are made public. This includes manuscript files, peer review comments, author rebuttals and revised materials. Note: This was optional for articles submitted before 13 February 2023.
Peer reviewers are encouraged (but not required) to provide their names to the authors when submitting their peer review. If they agree to provide their name, then their personal profile page will reflect a public acknowledgment that they performed a review (even if the article is rejected). If the article is accepted, then reviewers who provided their name will be associated with the article itself.
No comment
No comment
I was able to reproduce the examples on Linux with the provided Docker containers and macOS (I unfortunately do not have access to Windows), for Python 2 and 3 (except for the StructureAnalysis example on Python 3, as noted in the response and paper). A few of the conda recipes required minor modifications to work in macOS. I have made pull requests to the STL, ClangLite, and StructureAnalysis repos to fix these issues. These require no changes to the paper itself.
The (one) referee has a minor suggestion and also reports problems running the examples. Please consider that suggestion and see if you can address the problem experienced with the examples.
The paper itself is much improved. I only have one suggestion, which is to use the names of the wrapped libraries in the left column of the added table (table 3), instead of just section numbers.
I am happy to see that AutoWIG now supports Python 3, and am also happy to see the license change to a more standard licence. The examples are now much easier to see and run that they are in a separate repo. The fact that the generated files are untracked, along with the git status outputs in the notebooks, makes it clearer which files are generated.
Unfortunately, I was not able to get the example notebooks to run on my Mac. Each notebook failed with errors like
ImportError: dlopen(/Users/aaronmeurer/anaconda3/envs/FP17/lib/python3.6/site-packages/clanglite/__clanglite.so, 2): Library not loaded: @rpath/libboost_python.dylib
Referenced from: /Users/aaronmeurer/anaconda3/envs/FP17/lib/python3.6/site-packages/clanglite/__clanglite.so
Reason: image not found
This happened even if I skipped the conda build steps and used the package from conda-forge. It seems the libboost conda package used by the examples does not actually contain libboost_python.dylib. I do not have enough experience with boost-python to offer a solution, although I did notice that the boost conda package (from conda-forge) has libboost_python3.dylib.
Incidentally, it may be a good idea to create an environment.yml that locks in specific package versions known to work with each platform (conda env export -n fp17) for the purposes of reproducing the paper.
In the Python 3 docker container, both for the 1.0 and "latest" tagged containers, dependent.ipynb failed when building python-stat-tool with
In file included from build/src/py/wrapper/_stat_tool.h:6:0:
/home/main/miniconda/conda-bld/python-stat_tool_1509148131720/_b_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeh/include/stat_tool/reestimation.h: At global scope:
/home/main/miniconda/conda-bld/python-stat_tool_1509148131720/_b_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeh/include/stat_tool/reestimation.h:177:3: error: 'STAT_TOOL_API' does not name a type
STAT_TOOL_API double interval_bisection(Reestimation<double> *distribution_re
^
scons: *** [build/src/py/wrapper/_stat_tool.h.gch] Error 1
The other three notebooks worked in Docker under both tags.
It's also worth pointing out that the commands in the notebooks that use ! do not cause the notebook cell with the command to fail when they exit nonzero, meaning errors are typically seen in later cells.
I do not have a Windows machine, so I was unable to test that platform. However, I see that the notebooks are built on AppVeyor, but I was not able to find the builds.
The Binder link failed with
Step 27/28 : RUN conda env update -n root -f "environment.yml" && conda clean -t
ipsy
---> Running in ad47c3a7b88b
Fetching package metadata .............
Solving package specifications:
NoPackagesFoundError: Dependencies missing in current linux-64 channels:
- statiskit-toolchain -> libboost
- statiskit-toolchain -> python-clanglite -> libclanglite
Thank you for this submission. The two referees have provided very thorough reports. Please can you provide with your revision a detailed explanation of how you responded to each comment.
Please note that on line 21 the word "MATLAB" should be capitalized.
There are some minor grammar issues throughout the manuscript:
1. Missing articles: lines 119 ("a C++ library"), 150 ("the AutoWIG big
picture"), 212 ("the definition"), 288 ("the previous files"), 212 ("in
which the definition"), 288 ("class selection for the previous files"), 337
("a few problems"), 341 ("if the static overload"), 370 ("most of the
AutoWIG guidelines"), and 547 ("the C++11 standard") (also note "C++11"
should be spelled without any spaces) (note this list is not exhaustive).
2. Incorrect pluralization: lines 152 ("represents code abstractions and
captures code components"), 273 ("name collisions in Python"), Table 2
("this enables the choice"), 351, ("it eases code introspection"), Figure 2
(user tries to set"), 442 ("tries to convert"), 513 ("every type and
function has to be"), 582 ("AutoWIG translates"), and 584 ("used to
respectively generate"), (note this list is not exhaustive).
3. Some adverbs should go before the word they modify: lines 259
("Boost.Python extensively uses"), 282 ("This allows to easily select"),
and 442 ("automatically convert").
4. Some other minor grammar issues: lines 20 ("lower computer processing time
at the cost of high scientist coding time"), 78 ("even if there exists
classical approaches"), 281 (delete "contains"), Table 2 ("Three plugin
managers") (numbers that being a sentence should be spelled out), 307,
delete the comma ("Any non self-contained headers should not be given"),
334 ("in a basic Python package"), 341 ("has for its first parameter"), 342
("all following parameters correspond to another"), Figure 2 ("if a user"),
and 400, the sentence "assuming that the asg ..." is a fragment.
5. Line 21 "at the opposite, scripting languages" should perhaps be "at the
opposite end of the spectrum, scripting languages".
6. The way in which the sentence in lines 211-214 is written makes it very
difficult to read. I would suggest rewriting it as "In order to have access
to all template class specialization definitions, AutoWIG parses a virtual
program of undefined template class specialization definitions (e.g., using
sizeof(std::vector< int >); for forcing a std::vector< int > definition)."
7. The double negative in line 291 ("are also wrapped if they are not
explicitly marked as non-exportable") makes it difficult to parse. I would
suggest, "are also wrapped, unless they are explicitly marked as
non-exportable."
8. The sentence on lines 514-515, "note that if this approach..." is
confusing. The "if" in particular is confusing, as there doesn't seem to be
any logical causation to the implication. Perhaps (if I am correctly
understanding the authors' intention), a better reading would be, "note
that although this approach is only efficient when exposing a small number
of functions and objects, it is at the basis of all wrapper tools that
generate C API code".
9. The citation for (Perez and Granger, 2007) on line 598 is not parenthesized
correctly.
10. Please add a reference citation for abstract semantic graphs.
11. Are there references that can be added for StatTool and SequenceAnalysis?
12. Please add URLs for the software citations.
1. For the requirements section, there is no discussion about types and data
structures. This is distinct from the Pythonic interface. C++ is a typed
language, whereas Python is dynamic. Furthermore, Python has different
builtin data types from C++. So any wrapper must answer some questions: how
are invalid input types handled (C++ disallows this at compile time, but in
Python any type checks must happen at runtime)? How are Python types such
as int converted to the respective C++ types, and is any attempt made to
preserve Python semantics where they differ from C++'s (e.g., Python ints
are big integers which can be arbitrarily large)? And how are data
structures handled (will a wrapped function that requires an array or
vector type support a homogeneous Python list type? Will it support a NumPy
array? Can a wrapped function that operates in-place operate on a NumPy
array without copying)? I'm particularly interested in the last point. If a
wrapper cannot interoperate efficiently with NumPy arrays it will not be
useful for most scientific applications. My understanding is that AutoWIG
relies on Boost.Python to handle this logic.
This is discussed in the findings itself, in section 6.3, although it is
not clear what containers are supported, and in particular NumPy arrays are
never mentioned.
1. I did not see the code or data for the example in section 6.4 in the
autowig repository. If they are in another repository it should be listed
in the supplementary materials. As it stands this example cannot be
reproduced.
2. It is disappointing to see a new Python library that does not support
Python 3. Python 2 support will be sunsetting in 2020, which includes a
large number of scientific libraries (see
http://www.python3statement.org/). I have seen in the issue tracker that
this is due to the dependency on scons. In the manuscript the fact that the
library requires Python 2.7 is not mentioned until the end (in section
7.4), and no reason is given.
3. In the examples in the docs, the generated files are all checked in to the
git repository. This makes it difficult to tell which files are supposed to
be human generated and which are supposed to be machine generated.
4. I found it somewhat confusing/misleading that the intro materials of the
paper discuss C, C++, and Fortran, but AutoWIG itself only supports C++. I
understand that AutoWIG is designed in such a way that it could support
other languages, but that they are not presently supported was not clear
until the very end, when it was explicitly stated in section 7. The same
applies for R as a target language.
5. I'm curious what the largest pre-existing C++ library is that AutoWIG has
been used or attempted to be used on. No indication is given for how large
the examples from section 6 are, or if wrapping other larger libraries was
attempted.
6. Are the plots from section 6.4 generated with matplotlib as implied (lines
460 and 476 and Figures 3 and 4)? If so, how?
7. It would be helpful to see some example of what the Mako templates look
like, perhaps either as a suggestion of what they might look like for R in
section 7.2, or show part of the existing templates for Python in section
4.
8. Section 7.5 (line 608) mentions a tool for parsing compiler errors. Does
this tool actually exist in AutoWIG, or is this a proposal for future work?
If it does exist, it should be mentioned earlier. If it doesn't that should
be made clearer.
The AutoWIG approach to binding generation is promising. I like that it 1) is
designed to be used interactively, and 2) is designed to be used as a Python
library, without inventing a DSL.
1. On line 270, what is the "global name hash"? Is this a reference to the
Python built-in function "hash" or something else? If it is, will AutoWIG
still function correctly if hash randomization is enabled (it is on by
default in Python 3, and can be enabled in Python 2 using the -R flag to
the interpreter)?
2. The sentence on line 307, "any non self-contained headers should not be
given to a parser but can nevertheless be considered during parsing using
relevant search path flags (given by the -I option)," appears
contradictory, especially since the search path flags are one of the
arguments to a parser plugin. I cannot suggest an alternative because I am
really unclear what the authors are trying to say here.
3. It is stated that certain components are not wrapped due to ambiguity (line
322). Are these ambiguities warned about? How can they be worked around?
4. Is it necessary to set the "default" plugins (such as
autowig.controller.plugin = 'default', line 371)?
5. Concerning the plugin model, what is the value of the indirection
autowig.controller['clanglite'] = clanglite_controller
autowig.controller.plugin = 'clanglite'
asg = autowig.controller(asg)
rather than simply
asg = clanglite_controller(asg)
6. Does AutoWIG write a setup.py or any other build scripts, or does the user
need to write those manually?
7. Section 6.2 is an interesting meta-example (indeed, I believe this same
example was mentioned in Anthony Scopatz's 2013 SciPy presentation on
XDress as an interesting potential thing to wrap). However, since it is a
meta-example, it is unclear from the outset that this is indeed an example
and simply a discussion of AutoWIG's limitations, so I would suggest to
make this clearer somehow.
8. GCC-XML is dismissed because it does not support C++11. Would LLVM still be
preferred over it if it did support C++11?
9. The link to PySTL in the template.ipynb notebook
(https://github.com/StatisKit/PySTL) is broken. The link in the code
(https://github.com/StatisKit/STL) appears to be correct.
10. The conda build commands in the subset.ipynb file failed for me with
errors like
failed to get install actions, retrying. exception was: The following specifications were found to be in conflict:
- python 3.5.*
- python-dev -> libdev -> python 2.7*
Use "conda info <package>" to see the dependencies for each package.
Everything else in the notebook worked for me, except for some empty
warning message:
/Users/aaronmeurer/anaconda3/envs/autowig/lib/python2.7/site-packages/clanglite/autowig_parser.py:860: Warning:
warnings.warn('', Warning)
I believe it all ended up using my installed version of clanglite instead
of the source version, because the build step failed.
11. The subset.ipynb file has a typo that prevents it from running. 'from path
import path' should be 'from path import Path' in the clanglite_controller
function.
The paper is well presented. It researches literature and mentions all major
approaches to wrap C++ in Python. Then it describes a new approach based on
LLVM, which is more automatic and can be used with large and complex C++
libraries. The paper describes well how the AutoWIG works and how particularly
difficult C++ constructs are handled and how automatic documentation is
generated. It gives three bigger examples how the library can be used and the
corresponding Jupyter notebooks are available.
A minor correction:
1) e.g std::vector, std::set -> e.g., std::vector, std::set
I have downloaded the Docker image using the following command on Ubuntu 16.04:
$ docker run -i -t -p 8888:8888 statiskit/autowig:v1.0.0-alpha
And as instructed, I launched the Jupyter notebook from inside the container
using:
$ jupyter notebook --ip='*' --port=8888 --no-browser
The `basic.ipynb` ran fine. Here are some minor corrections that I noticed:
2) a basic example of C++ library -> a basic example of a C++ library
3) A `basic.ProbabilityError` should have been raise -> raised
The `subset.ipynb` failed in the cell [7] with the following error:
```
---------------------------------------------------------------------------
ImportError Traceback (most recent call last)
<ipython-input-7-4e28706c39ff> in <module>()
----> 1 get_ipython().run_cell_magic(u'time', u'', u"autowig.controller['clanglite'] = clanglite_controller\nautowig.controller.plugin = 'clanglite'\nasg = autowig.controller(asg)")
/home/main/miniconda/lib/python2.7/site-packages/IPython/core/interactiveshell.pyc in run_cell_magic(self, magic_name, line, cell)
2115 magic_arg_s = self.var_expand(line, stack_depth)
2116 with self.builtin_trap:
-> 2117 result = fn(magic_arg_s, cell)
2118 return result
2119
<decorator-gen-60> in time(self, line, cell, local_ns)
/home/main/miniconda/lib/python2.7/site-packages/IPython/core/magic.pyc in <lambda>(f, *a, **k)
186 # but it's overkill for just that one bit of state.
187 def magic_deco(arg):
--> 188 call = lambda f, *a, **k: f(*a, **k)
189
190 if callable(arg):
/home/main/miniconda/lib/python2.7/site-packages/IPython/core/magics/execution.pyc in time(self, line, cell, local_ns)
1183 else:
1184 st = clock2()
-> 1185 exec(code, glob, local_ns)
1186 end = clock2()
1187 out = None
<timed exec> in <module>()
<ipython-input-6-316a91bf6e70> in clanglite_controller(asg)
132
133 import sys
--> 134 from path import path
135 for header in (path(sys.prefix)/'include'/'clang').walkfiles('*.h'):
136 asg[header.abspath()].is_external_dependency = False
ImportError: cannot import name path
```
The `template.ipynb` failed in cell [5] with:
```
scons: Entering directory `/home/main/AutoWIG/doc/examples/STL'
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Cleaning targets ...
Removed /home/main/miniconda/include/statiskit/stl/STL.h
scons: done cleaning targets.
scons: Entering directory `/home/main/AutoWIG/doc/examples/STL'
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
Install file: "build/src/cpp/STL.h" as "/home/main/miniconda/include/statiskit/stl/STL.h"
Install file: "build/src/cpp/AutoWIG.py" as "/home/main/miniconda/lib/python2.7/site-packages/autowig/site/controller/statiskit_stl.py"
autowig: Generating Boost.Python interface ...
scons: *** [/home/main/miniconda/lib/python2.7/site-packages/autowig/site/ASG/statiskit_stl.pkl] ImportError : No module named clanglite
Traceback (most recent call last):
File "/home/main/miniconda/lib/python2.7/site-packages/SCons/Action.py", line 1054, in execute
result = self.execfunction(target=target, source=rsources, env=env)
File "/home/main/miniconda/lib/python2.7/site-packages/SCons/site_scons/site_tools/wig.py", line 38, in boost_python_builder
parser = import_module('autowig.site.parser.' + AUTOWIG_PARSER)
File "/home/main/miniconda/lib/python2.7/importlib/__init__.py", line 37, in import_module
__import__(name)
ImportError: No module named clanglite
scons: building terminated because of errors.
```
Some corrections:
4) Once the headers habe been installed in the system -> have been
5) situed -> situated
6) For the complete procedure refers to the -> refer
7) Once these preliminaries done -> are done
The package has a simple test suite and it is tested by Travis-CI. The fact
that two out three notebooks failed for me in Docker (which should be very
reproducible) suggests that the installation and usage procedure is somewhat
fragile.
8) It would be nice to clarify exactly which Docker tag is supposed to work and
clarify in the documentation the exact steps to make all three notebooks
succeed.
The package seems to work, with the caveats in the previous section. The main
conclusion that AutoWIG greatly simplifies the process of wrapping large and
complex C++ libraries seems to be true. In order to increase the adoption of
the package, I provided a note about a license below, that the authors might
optionally consider.
I recommend the article for publication, after the minor changes 1)-8) are
fixed.
License: currently the AutoWIG software is distributed using the CeCILL-C
license. The Python scientific community encourages to use permissive licenses
such as MIT or BSD [1], and most packages are licensed as such, e.g., from the
libraries the authors cited in the paper, the following use permissive
licenses: Python, SciPy, Cython, Boost.Python, SWIG, F2PY, XDress, IPython,
Jupyter, LLVM, Clang, Armadillo, Matplotlib, VTK, Mako, libc++ (STL), Sphinx,
WrapITK, Docker. The libraries that do not use permissive open source licenses:
Sage, Matlab, SWIG, R, Octave, Rcpp, Doxygen. The authors might consider using
a permissive license such as MIT and BSD to get wider adoption of the library
by the scientific Python community. However, this is by no means an obstacle to
publishing this article, and also the authors might not be in a position to
change the license in the first place.
[1] http://nipy.sourceforge.net/software/license/johns_bsd_pitch.html
All text and materials provided via this peer-review history page are made available under a Creative Commons Attribution License, which permits unrestricted use, distribution, and reproduction in any medium, provided the original author and source are credited.