-
-
Notifications
You must be signed in to change notification settings - Fork 316
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
pygrass: add get_json_dict function #2937
base: main
Are you sure you want to change the base?
Conversation
@pesekon2 Do you have any opinion on the addition? You added actinia export to the modeller lately... |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Module( | ||
"r.slope.aspect", | ||
elevation="elevation", | ||
slope="slope", | ||
aspect="aspect", | ||
overwrite=True, | ||
verbose=True, | ||
run_=False, | ||
).get_json_dict(export="GTiff") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing against smoke tests in general, but these tests are unnecessary simple. Please add assertions to move it beyond smoke tests. It does not really tell what is the expected output.
@@ -53,6 +53,44 @@ def test_rsun(self): | |||
out.close() | |||
|
|||
|
|||
class TestModulesJsonDictExport(TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using pytest for these test. No data needed. Assert functionality suited for for plain values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we can run them faster/in parallel! (I was exploring making pytest run for windows and macOS the other day, but still had problems making it find the pytest I installed on the windows CI)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case is now removed and replaced with pytest in new file: pygrass_modules_interface_json_test.py
which has assertations. Hope that is roughly along your lines of thought...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the actual concept here, why this is actually needed? We already have the same JSON emitted from the tools themselves with --json
:
grass --tmp-project XY --exec python
>>> import subprocess
>>> import json
>>> json.loads(subprocess.run(["r.info", "map=elevation", "--json"], capture_output=True).stdout)
{'module': 'r.info', 'id': 'r.info_1804289383', 'inputs': [{'param': 'map', 'value': 'elevation'}]}
This PR's code:
>>> from grass.pygrass.modules.interface import Module
>>> Module("r.info", map="elevation", run_=False).get_json_dict()
{'module': 'r.info', 'id': 'r_info_bb8b711437c149fdb00e1a4ccdb77839', 'inputs': [{'param': 'map', 'value': 'elevation'}]}
Difference:
grass --tmp-project XY --exec r.info map=elevation --json | jq > old.json
grass --tmp-project XY --exec python -c \
'from grass.pygrass.modules.interface import Module; import json; print(json.dumps(Module("r.info", map="elevation", run_=False).get_json_dict()))' \
| jq > new.json
diff -u old.json new.json
{
"module": "r.info",
- "id": "r.info_1804289383",
+ "id": "r_info_4722a5e5637f47cb8bf561591f8a4435",
"inputs": [
{
"param": "map",
Valid question. My use case for that function is prototyping actinia workflows in jupyter.
Overall, I think the get_json() function is more user-friendly, at least in the context of my usecase than the |
Why you just don't call the tools with As far as the current state, you talk about interface and functionality, but I'm concerned with maintenance. If this would be merged, we would be left with maintenance of two JSON generators which are actinia-specific (so need to at least somewhat follow changes in actinia) and the JSON format is not well specified, at least not within GRASS GIS, further hindering the maintenance.
The Bash and Python functions don't duplicate any substantial code in the library. get_json does. More generally, I was wondering if you even know about |
I would not be disappointed if the decision is not to merge this. I can add that function to the Module object in my downstream code... No problem. So please feel free to close this as wontfix, if that is preferable... |
Why don't you just call the tool with Check other recent grass.pygrass PRs or issues. The overall issue there is that it tries to mimic what the actual command line parser is doing, but it fails for edge cases. Using the tool to figure out these cases would be a better solution, but it requires more substantial rewrite, so I was just doing smaller fixes here and there. Using |
This PR adds a
get_json_dict
function to the pygrass Module interface.Motivation is to make it easier to create JSON intput to the actinia REST API for GRASS GIS.
Here is a usage example:
Suggestions for changes are most welcome...