-
-
Notifications
You must be signed in to change notification settings - Fork 360
grass.experimental: Add object to access tools as functions #2923
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
base: main
Are you sure you want to change the base?
Conversation
It seems to be an useful addition. On the other hand we have already two APIs to run GRASS modules: |
I still need to provide more context for this, but do you see some benefits already?
The intro to this is obviously xkcd Standards. I'm not happy with the two competing interfaces. It's almost three, because we have Module and than also shortcuts. As far as I understand,
The design idea is 1) to make the module (tool) calls as close to Python function calls as possible and 2) to access the results conveniently. To access the (text) results, it tries to mimic subprocess.run. Additionally, it tries to 1) provide consistent access to all modules and 2) allow for extensibility, e.g., associating session parameters or computational region with a Tools object rather than passing it to every method. The existing APIs are more general in some ways, especially because they make no assumptions about the output or its size. This API makes the assumption that you want the text output Python or that it is something small and you can just ignore that. If not, you need to use a more general API. After all, Tools itself, is using pipe_command to do the job.
Given the different goals of the two APIs, I was not able to figure out how these can be merged. For example, the Module class from grass.pygrass was supposed to be a drop-in replacement for run_command, but it was not used that way much (maybe because it forces you to use class as an function). Any suggestions? What would be the features and aspects of each API worth keeping? For example, the Tools object might be able to create instances of the Module class. I can also see that some parts of the new API could be part of the old ones like output-parsing related properties for the Module class, but there are some existing issues which the new API is trying to fix such as Finally, the subprocess changed too over the years, introducing new functions with run being the latest addition, so reevaluation of our APIs seems prudent even if it involves adding functions as subprocess did. Anyway, I think some unification would be an ideal scenario. |
96b1d0c
to
0c21f1a
Compare
This is how exceptions look like currently in this PR: The error (whole stderr) is part of the exception, i.e., always printed with the traceback, not elsewhere, and it is under the traceback, not above like now (or even somewhere else in case of notebooks and GUI).
|
This adds a Tools class which allows to access GRASS tools (modules) to be accessed using methods. Once an instance is created, calling a tool is calling a function (method) similarly to grass.jupyter.Map. Unlike grass.script, this does not require generic function name and unlike grass.pygrass module shortcuts, this does not require special objects to mimic the module families. Outputs are handled through a returned object which is result of automatic capture of outputs and can do conversions from known formats using properties. Usage example is in the _test() function in the file. The code is included under new grass.experimental package which allows merging the code even when further breaking changes are anticipated.
…ute with that stdin
7996926
to
24c27e6
Compare
Solved conflicts |
Since somewhere during the code sprint, in the PR that updated markdown lint in pre-commit, I had to fix this usability problem as the Ubuntu 22.04 runners defaulted to node 18 (others are installed but not active), but is already EOL or very close to, and the tool dropped support since. I configured it in a way that most normal users won't need to do anything or think about. Pre-commit uses the system node if available, otherwise sets up one in an environment similar to venvs for python. If you would to try again today (with the pre-commit file from the main branch, or the branch updated), it should work as expected. |
…that the type is float
…ng on Windows. Although the subprocess.run call is much nicer, whatever is necessary for the subprocess itself, good or bad, is contained in gs.Popen. This makes it consistent with the actual tool call later on. We may need a library wrapper for this in the future.
…run result, default to text mode
…workaround the tool help by manually handling the argparse help, but only for the run subcommand
Experimental CLIThis is not really that important for this PR specifically, but similarly to what I did when adding mapset locking to the Python library, I added CLI for testing. Set up needed without FHS: export PYTHONPATH=$(./bin.x86_64-pc-linux-gnu/grass --config python-path) There is only temporary XY project set up, so it is not really useful at this point, except for things like help or g.extension: python -m grass.app run g.extension -l
python -m grass.app run g.region --help
python -m grass.app run r.slope.aspect --interface-description I'm using the CLI as an unusual use case for the Tools API, so it helps me understand that standard input and output work as expected, and they do: # stdin
echo "a = 6" | python -m grass.app run r.mapcalc file=-
# stdout
python -m grass.app run g.region -p | grep res The CLI will become more interesting over time with changes such as #5877 and #5843. |
… the error message which may get automatic suggestion by Python when __dir__ is now in place.
…me, count on Python possibly adding a sentence), use g.region rather than g.search.modules output in tests because the output is more predictable (although we loose the JSON test without format=json)
… allows for the changes in the session to be reflected in the tools. This requires overwrite and verbosity setting to be handled in an internal, adhoc copy which is avoided if not needed. No special computational region treatment.
…ad, fully rely on io.StringIO. This assumes that any tool which will have stdin will have something like input=- or file=- because there is no other way of getting stdin data to the tool at this point.
…more general to accomodate future additions such as standalone tools. Use lazy imports to enable from ...tools import Tools without forcing that import on everyone importing anything from the other modules. Use short names for the run functions to always keep the focus on the tool. Distinguish smart and basic by run and call, and parameter-based and list by _cmd suffix.
This adds a Tools class which allows to access GRASS tools (modules) to be accessed using methods. Once an instance is created, calling a tool is calling a function (method) similarly to grass.jupyter.Map. Unlike grass.script, this does not require generic function name and unlike grass.pygrass module shortcuts, this does not require special objects to mimic the module families.
Outputs are handled through a returned object which is result of automatic capture of outputs and can do conversions from known formats using properties.
Usage example is in the _test() function in the file.
The code is included under new grass.experimental package which allows merging the code even when further breaking changes are anticipated.