ubarsc / python-fmask

A set of command line utilities and Python modules that implement the ‘fmask’ algorithm
https://www.pythonfmask.org
GNU General Public License v3.0
74 stars 21 forks source link

Remove subprocess calls and use GDAL utility functions #64

Closed gillins closed 1 year ago

gillins commented 1 year ago

Should be a bit tidier, especially on Windows.

neilflood commented 1 year ago

Sigh..... Turns out this creates a problem. It looks like the gdal_merge.main() function opens the output file using gdal.GA_Update. Because we create the file before the call, using mkstemp(), it already exists, and so tries to open an existing file. However, it is an empty file, so that open fails.

Not entirely sure what to do. We could create the file with mkstemp(), then remove it and keep the name it generated, I guess, although this is not quite in the spirit of mkstemp().

What do you think?

gillins commented 1 year ago

Oh dear. So it worked before because it was in a different process?

Yes remove the file first I reckon, yes not quite in the spirit but what can you do. I'll do that now.

neilflood commented 1 year ago

OK, now it gives a slightly different error, about the file not existing. Weird. Might be a timing thing with the removed file still being visible for a millisecond.

I have just trawled through the code for gdal_merge.py (line 448). It looks like it should work without us having to remove the file. It attempts the Open on an existing file, and if that fails it is supposed to Create it.

However, when running it within our process, the first attempt produces an exception and just fails out. My guess now is that we somehow have UseExceptions active, and its own mechanism for coping with that may not be coping.

I think I will come back to this tomorrow.......... Leave it with me.

neilflood commented 1 year ago

So, firstly, I can confirm that this problem is arising due to having UseExceptions on. If I kludge a DontUseExceptions() call into gdal_merge.py just before the Open, then the problem goes away.

However, even after removing all the UseExceptions() calls from within python-fmask, we still have it on. It looks like there is something doing it within RIOS. If one just does

>>> from rios import applier
>>> from osgeo import gdal
>>> print(gdal.GetUseExceptions())
1

we see that it is being turned on somewhere inside RIOS.

So, we should probably try and find that, too. In the meantime, we could make this work here by saving the exception state just before these gdal_merge sections of code. I will have a go.......

gillins commented 1 year ago

I don't think we did a RIOS release since we fixed a bunch of these (https://github.com/ubarsc/rios/commit/69f6266235440b74eef7247847b808b68aaaa961). Might work if you install RIOS from master?

neilflood commented 1 year ago

Ah, that's good to know.

However:

In view of these points, I think we had better make these gdal_merge.main calls robust against exceptions being on. It seems that it really doesn't cope at all, so they need to be off at the point when it is called. So, I will push ahead with that anyway. Stand by for a push.....

neilflood commented 1 year ago

OK, that all seems to work now, at least on Linux. I think you can merge that when you are happy.

gillins commented 1 year ago

Thanks Neil!