Opened 19 months ago

Closed 14 months ago

Last modified 14 months ago

#3537 closed enhancement (fixed)

Add functionalities –tmp-location and –no-clean

Reported by: radeknovotny94 Owned by: grass-dev@…
Priority: normal Milestone: 7.6.0
Component: Startup Version: svn-trunk
Keywords: tmp-location, no-clean, exec Cc:
CPU: All Platform: All

Description

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

Attachments (4)

grass-tmp-loc.diff (8.9 KB) - added by radeknovotny94 19 months ago.
grass-tmp-loc-2.diff (10.2 KB) - added by radeknovotny94 19 months ago.
grass.py_check_geostring.patch (1.3 KB) - added by Nikos Alexandris 14 months ago.
Check if geostring exists and if target file is readable
grass.py_check_geostring.2.patch (1.2 KB) - added by Nikos Alexandris 14 months ago.
Check if geostring exists and if target file is readable

Download all attachments as: .zip

Change History (15)

comment:1 Changed 19 months ago by 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.

comment:2 Changed 19 months ago by martinl

Milestone: 8.0.07.6.0

Changed 19 months ago by radeknovotny94

Attachment: grass-tmp-loc.diff added

comment:3 in reply to:  1 Changed 19 months ago by 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')

comment:4 Changed 19 months ago by 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?
  1. Please provide documentation for this, ideally as part of the grass7.html file. This will make testing easier and addressing the use cases.
  1. 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.
  1. 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.
  1. 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.

comment:5 in reply to:  4 Changed 19 months ago by 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.

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

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

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

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

Changed 19 months ago by radeknovotny94

Attachment: grass-tmp-loc-2.diff added

comment:6 Changed 17 months ago by 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.

comment:7 Changed 14 months ago by wenzeslaus

Resolution: fixed
Status: newclosed

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.

comment:8 Changed 14 months ago by 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.2.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 https://trac.osgeo.org/grass/attachment/ticket/3537/grass.py_check_geostring.patch:

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

Last edited 14 months ago by Nikos Alexandris (previous) (diff)

Changed 14 months ago by Nikos Alexandris

Check if geostring exists and if target file is readable

Changed 14 months ago by Nikos Alexandris

Check if geostring exists and if target file is readable

comment:9 Changed 14 months ago by 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.

comment:10 in reply to:  9 Changed 14 months ago by 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).

comment:11 Changed 14 months ago by 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.

Note: See TracTickets for help on using tickets.