Add functionalities –tmp-location and –no-clean

[radeknovotny94]

Implementation of –-tmp-location, which extends functionality –exec. Creates new temporary location, where takes place the computation from –exect and after that is location deleted.

Second functionality –no-clean arranges that after computation location and temporary files are not deleted.

See ​https://trac.osgeo.org/grass/wiki/GSoC/2018#Neweasy-to-useCLIandAPIforGRASSGIS

attachment:grass-tmp-loc.diff​

[wenzeslaus]

Thumbs up for creating a find_tmp() function, but you replaced:

for ttmp in ("/tmp", "/var/tmp", "/usr/tmp"):
    tmp = ttmp
    tmpdir = os.path.join(tmp, "grass7-%(user)s-%(lock)s" % {
                                      'user': user, 'lock': gis_lock})
    try:
        os.mkdir(tmpdir, 0o700)
    except:
        tmp = None
    if tmp:
        break

by:

def find_tmp():
    ...
    for ttmp in ("/tmp", "/var/tmp", "/usr/tmp"):
        tmp = ttmp
...
tmp = find_tmp()
tmpdir = os.path.join(tmp, "grass7-%(user)s-%(lock)s" % {'user': user,
                                                         'lock': gis_lock})
try:
    os.mkdir(tmpdir, 0o700)
...

Note esp. the for loop and the test using try.

[radeknovotny94]

Replying to wenzeslaus:

Thumbs up for creating a find_tmp() function, but you replaced
...
Note esp. the for loop and the test using try.

I looked at it again and I think that finally is all new function find_tmp unnecessary. Because it is set environmental variable TMPDIR at the end of function create_tmp.

# promoting the variable even if it was not defined before
os.environ['TMPDIR'] = tmpdir

It means that TMPDIR have to be defined when we call function for creating temporary location.

gisdbase = os.getenv('TMPDIR')

[wenzeslaus]

Thank you for submitting a new version. Perhaps next time, leave there the old diff file, so we can refer to it if needed. About the diff itself:

1. I didn't catch it the first time, but it seems there was misunderstanding about what --no-clean should do. See the wiki again, ...if --exec should clean the .tmp directory in the Mapset. This relates to the clean_temp() function (it is not connected to the Cleaner class, the code is only partially re-factored and this is really something different anyway). The description at the wiki says for parallel execution. Try to run two (longer running) processes in parallel, at least sometimes, one will fail because the other deleted its temporary files in the Mapset .tmp directory. Let us know if you need more explanation on this, perhaps directly on the mailing list, so that more people can answer. I'm not sure at this point if your --no-clean is useful for something or not. Did you have any use case?

2. Please provide documentation for this, ideally as part of the grass7.html file. This will make testing easier and addressing the use cases.

3. It would be great of you come up with some more formal tests for it. It does not really fit into the gunittest tests but we can always start with a Bash script executing series of commands.

4. Any particular reason to place --no-clean on the same line with [[[GISDBASE/]LOCATION_NAME/]MAPSET] in the help message? It should be probably with --exec or close to -f.

5. In generally, please avoid unrelated whitespace changes in the patches/commits. Whitespace and other PEP8 things can and should be changed, but in separate commits. See your lines 375 and 403 (Trac view numbering) in your diff.

[radeknovotny94]

Replying to wenzeslaus:

Thank you for submitting a new version. Perhaps next time, leave there the old diff file, so we can refer to it if needed. About the diff itself:

1. I didn't catch it the first time, but it seems there was misunderstanding about what --no-clean should do. See the wiki again...

Thank you for the better explanation, I tried to fix it in new patch. Please add your comments.

2. Please provide documentation for this, ideally as part of the grass7.html file. This will make testing easier and addressing the use cases.

I made the first version of the documentation and I am also waiting for your comments.

3. It would be great of you come up with some more formal tests for it. It does not really fit into the gunittest tests but we can always start with a Bash script executing series of commands.

I will make this Bash script in next days.

4. Any particular reason to place --no-clean on the same line with [[[GISDBASE/]LOCATION_NAME/]MAPSET] in the help message? It should be probably with --exec or close to -f.

It was because in my previous implementation --no-clean works for calling without --exec too. I changed it.

5. In generally, please avoid unrelated whitespace changes in the patches/commits. Whitespace and other PEP8 things can and should be changed, but in separate commits. See your lines 375 and 403 (Trac view numbering) in your diff.

Thanks, I know it, I was more attentive about it this time.

[wenzeslaus]

I committed a modified --tmp-location portion of the patch (grass-tmp-loc-2.diff​) in r72790. I provided couple use cases in the documentation.

I worked on the --no-clean portion a little bit, but that needs more work. The mapsets .tmp directory is deleted at many places.

[wenzeslaus]

The main part, --tmp-location, was implemented in r72790 and improved in r73096 (#3585), 73099, and r73098 (#3586). Now we can do:

Execute script in a loc created based on EPSG:

grass75 --tmp-location EPSG:3358 --exec script.py

Get CRS for a file:

grass75 --tmp-location ~/data/elevation.tiff --exec g.proj -p

Get help text for a module:

grass75 --tmp-location XY --exec r.neighbors --help

--no-clean is a case for a separate ticket (please open it if you are interested).

Closing this one as fixed.

[Nikos Alexandris]

A suggestion to check for if the 'geofile' (in grass.py named as geostring) exists and if it is is readable: ​https://trac.osgeo.org/grass/attachment/ticket/3537/grass.py_check_geostring.patch.

I believe to have tested this properly. I could not follow what is recommended at ​https://stackoverflow.com/a/82852/1172302. In example, something like

import pathlib
try:
    geostring = pathlib.Path(geostring)
    geostring.resolve()
except IOError as e:
    fatal(_("{error}"
        "\n\n"
        "Check spelling "
        "or the file's read permissions.").format(error=e))
else:
    # create location using georeferenced file
    gcore.create_location(gisdbase, location,
            filename=geostring)

would not test for read access. I could find a way to test for read access using something from ​https://docs.python.org/3/library/pathlib.html.

Maybe this can be implemented by using "future" built-in exceptions as FileNotFoundError (see ​https://docs.python.org/3/library/exceptions.html)?

[Nikos Alexandris]

Check if geostring exists and if target file is readable

[Nikos Alexandris]

Check if geostring exists and if target file is readable

[Nikos Alexandris]

Just some extra thoughts: perhaps the error message should *not* say anything but what actually happens: the file is not found.

What are your thoughts in general? Even this very check, in case the file does exist, does not ensure that the file will be available. Right after the check anything can happen at the OS level.

[wenzeslaus]

Hi Nikos, I'm looking at this now and:

1) I can't reproduce the "if the file does not exist, the process hangs" behavior from your ​email. I get ERROR: ... xxx.xxx: No such file or directory.

2) This should be really a separate ticket because it is/would be a bug (this is a closed enhancement ticket).

3) It is unrelated to --tmp-location unless you can show that it does not happen with -c (important only for testing and reporting).

4) Your considerations about what to report based on what are the right thing to do, let's discuss it in a new ticket of you can reproduce the hanging or you have some other issues (like the message).

[Nikos Alexandris]

I didn't open a new ticket for this because I (wrongly) thought it to be part of the implementation of --tmp-location. Will open one--though the problem might reside on my OS/File-System side.