Jason, thanks for the effort ! I didn't find any obvious errors and it will likely be very valuable to newcomers !
I just made a few changes in the way of expressing things. I guess some of the unpleasant effects you can observe are more likely side effects of the implementation and not really *design* decisions. That might be fixed/changed in the future, but it will be weighted against backward compatibility. The part where I'm not sure if about the Numpy ABI problems. I wasn't very aware of problems related to it. From what I've seen in the code, it looks like that the python bindings only use the C API/ABI of Numpy. But they indeed access numpy structures (struct PyArrayObject namely), so if those structures change between numpy versions, you're likely to have troubles. I don't believe it is related to the fact that the GDAL python bindings are in C or C++ : the swig generated files are indeed C++, but that's unrelated to the numpy ABI issue, IMHO. However, I haven't changed anything in this section as I'm not 100% sure of my analysis. Best regards, Even Le lundi 20 septembre 2010 18:29:26, Jason Roberts a écrit : > Even, > > Thank you very much for your comments. Based on them, I started a new > "Python Gotchas" page here http://trac.osgeo.org/gdal/wiki/PythonGotchas, > referenced from http://trac.osgeo.org/gdal/wiki/GdalOgrInPython. Please > review it to make sure I got everything right. > > Best regards, > > Jason > > -----Original Message----- > From: Even Rouault [mailto:[email protected]] > Sent: Saturday, September 18, 2010 5:52 AM > To: [email protected] > Cc: Jason Roberts > Subject: Re: [gdal-dev] With Python bindings, should geometry objects > created by ogr.Geometry() be explicitly destroyed? > > Jason, > > > I have some Python code that uses OGR geometry objects internally, > > creating them like this: > > > > > > > > point = ogr.Geometry(ogr.wkbPoint) > > > > > > > > Does this code need to explicitly destroy these geometries, like the > > following, to avoid leaks, or can it simply allow them to go out of scope > > and have Python's reference counting and garbage collector clean them up? > > > > > > > > point.Destroy() > > There's no reason to call Destroy(), at all. Native object gets destroyed > when Python object goes out of scope, or when they are assigned to None. > So replace foo.Destroy() by foo = None if you really want to control when > the underlying C++ object is destroyed. This is indeed generally only > interesting for I/O objects, like a GDALDataset or OGRDataSource, when you > want to control when the file handle is closed. > > ds = gdal.GetDriverByName('GTiff').Create('foo.tif', 10, 10, 1) > ds.GetRasterBand(1).Fill(255) > ds = None # Now, foo.tif is closed and I can safely copy it > shutil.copy('foo.tif', 'other.tif') > > > Does the situation change if I call feature.SetGeometry(point)? What > > about feature.SetGeometryDirectly(point)? > > No, you don't need to call Destroy() either. SetGeometryDirectly() brings a > bit of subtelty though : it makes the (C++) feature object the owner of the > (C++) geometry object (whereas SetGeometry() just clones the passed > geometry). The consequence for Python side is then when feature goes out > of scope, C++ feature object dies and the C++ geometry object too, so the > Python point object just referes to a dead C++ object. > > So do *NOT* do that : > > point = ogr.Geometry(ogr.wkbPoint) > feature = ogr.Feature(layer_defn) > feature.SetGeometryDirectly(point) > feature = None # point becomes implicitely invalid at that point > print point.ExportToWkt() # here you die > > The issue is that the bindings here expose something that is probably > manageable for C++ programmers used to take into account object lifetime, > but in Python, this can have surprising effect... > > > I'm sorry for my ignorance here. I found a nice GDAL tutorial that seems > > to say they *should* be explicitly destroyed in certain circumstances > > (see > > http://www.gis.usu.edu/~chrisg/python/2009/lectures/ospy_slides2.pdf, > > page 12). But I have not really seen any other examples of this. > > Destroy() was perhaps necessary with old-gen bindings, but I'm not even > sure of that... Perhaps this shouldn't have been exposed at all... But, as > mentionned in the slides, it is true that there are situations where you > shouldn't call Destroy() at all. > > Do *NOT* do this : > > feat = lyr.GetNextFeature() > geom = feat.GetGeometryRef() > geom.Destroy() # Arg! C++ feature still points at the underlying C++ > geometry, but is is now dead > feat = None # this will destroy the C++ feature, that will try to destroy > the C++ geometry, but this one is already destroyed --> segfault ! > > > If you follow gdal-dev, you probably notice that the "to destroy or not > > to destroy" question comes up periodically from Python programmers, as > > does the problem where someone gets a GDALDataset, gets a band, allows > > the GDALDataset to go out of scope, and then expects the band to still > > work. These are areas where the GDAL Python bindings to not work like > > other Python libraries, causing confusion among Python programmers, who > > generally expect reference counting that "just works" and that they only > > need to explicitly close something when some kind of I/O is involved. > > Agreed. > > I wouldn't say that is a problem of reference counting, but more a problem > of ownership and lifetime of C++ objects, that are sometimes destroyed > when a Python object get out of scope, but shouldn't since another Python > object indirectly still need them to be alive. > > Do *NOT* do this : > > feat = lyr.GetNextFeature() > geom = feat.GetGeometryRef() > feat = None # This kills the C++ feature, and its C++ geometry > print(geom.ExportToWkt()) # segfault here. The C++ geometry no longer exist > > Byt you could do (and this is what the patch in ticket 3592 I mention below > does under the hood): > > feat = lyr.GetNextFeature() > geom = feat.GetGeometryRef() > geom.please_keep_alive_object_after_equal_sign = feat > feat = None # The underlying C++ object is still alive, since geom keeps > the python feature object alive > print(geom.ExportToWkt()) > > > Similarly, do *NOT* do this : > > ds = gdal.Open('foo.tif') > band = ds.GetRasterBand(1) > ds = None > print(band.Checksum()) > > But you could do : > > ds = gdal.Open('foo.tif') > band = ds.GetRasterBand(1) > band.please_keep_alive_object_after_equal_sign = ds > ds = None > print(band.Checksum()) > > > Do *NOT* do this also : > > print(gdal.Open('foo.tif').GetRasterBand(1).Checksum()) > > > It would be really great if there was FAQ that pointed out all of the > > ways that GDAL differs from typical Python libraries. The scenarios I > > mention above seem to be two of the most common ones. Would someone be > > willing to create it? I would but do not feel competent enough at this > > point. > > If you start a wiki page collecting all the info you found in the mailing > list threads, that would be a beginning... > > Obviously, instead of documenting weird behaviour a better approach would > be to fix the code so it behaves as expected... > > There was an attempt I pursued in http://trac.osgeo.org/gdal/ticket/3592 , > that would solve all the crashes in the above examples, but as you can see > in the ticket, it raises backward compatibility issues in some cases > (including the GDAL/OGR python autotest suite)... So the question is a bit > like : should we inflict pain to existing users (in a few scenarios) to > make the life of new users easier... ? > > But the patch doesn't solve all cases. Unfortunately, there would be still > ways of causing segfault from Python. You can break some subtle API > contracts, like adding a new field to a layer when features deriving from > this layer definition are still active. And it isn't obvious how we could > detect such situations to prevent them from happening (see > http://trac.osgeo.org/gdal/ticket/3552 ) > > Do *NOT* do that : > > feature = lyr.GetNextFeature() > field_defn = ogr.FieldDefn("foo", ogr.OFTString) > lyr.CreateField(field_defn) # now, existing features deriving from this > layer are invalid > feature.DumpReadable() # segfault > > > Thanks, > > > > Jason _______________________________________________ gdal-dev mailing list [email protected] http://lists.osgeo.org/mailman/listinfo/gdal-dev
