grass.script.setup: Add return of a context manager from init

[albertoparadisllop]

Issue #1848

Turned init function into a class with __enter__ and __exit__ methods. Had to remove output.

I think its a bit of a weird solution, but this implementation allows for it to be used as a context manager, and as it has been used in the past (unless the return value from the function was used).

"Normal" use, almost same as before:

 # ... setup GISBASE and PYTHON path before import
import grass.script as gs
gs.setup.init("/usr/bin/grass8",
              "/home/john/grassdata",
              "nc_spm_08", "user1")
# ... use GRASS modules here
# end the session
gs.setup.finish()

As a context manager:

# ... setup GISBASE and PYTHON path before import
import grass.script as gs
with gs.setup.init("/usr/bin/grass8",
                    "/home/john/grassdata",
                    "nc_spm_08", "user1"):
    # ... use GRASS modules here
# session ended automatically

I tried to implement this with contextlib.contextmanager, but that would only allow it to be used as a context manager exclusively. If contextlib.contextmanager is the preferred solution, it'd have to be split into two separate functions, one for use as a context manager, and one as it has been used in the past.

Another approach could be to instead have the class use an open() and close() functions, and simply have the context manager call those automatically. This would mean they would have to be called explicitly in non-context-manager uses though.

Changed to use this approach. Created a SessionHandle class to be used as context manager, and with open() and close() methods. init not changed to ensure backwards compatibility.

I'm a beginner, so any feedback is welcome. Done for Hacktoberfest.

[wenzeslaus]

Thank you @albertoparadisllop. I'm afraid I agree that this is a weird solution. I think there are couple of changes which will make it better.

I'm not 100% convinced about that, but it seems to me that a good approach given the current code is to return a "session object" from the init function. See #1834 where I do something similar to what is needed here. Would a new class named, e.g., SessionHandle, which would be the context manager work?

Having also the manual open-close approach would be good to have. If we keep init usable as before, this may simplify things for many people and we don't need to try to push this to 8.0. In other words, keeping backwards compatibility would be very useful.

Additionally, I merged #1829, so it is probably easiest for you to start fresh from the main branch and open a new PR (unless you want to get into some Git tricks). The new code explains the return value or that we can/will change it.

[albertoparadisllop]

I replaced the previous changes with a separate class. This way the old init function is kept exactly the same, and backwards compatibility is preserved.

The class, SessionHandle simply calls init() on SessionHandle.open(), and finish on SessionHandle.close(). Those functions are automatically called on use as a context manager.

Both SessionHandle.open() and SessionHandle.__enter__() return gisrc to keep it as similar to init as possible, but perhaps that could be removed.

And finally, I considered adding a boolean opened as some sort of "lock", to avoid calling session.open() on an already-opened/unclosed session, but I wasn't sure what the way to deal with that would be. Perhaps some sort of exception, or ignorign it and writing to stderr. Or perhaps that would be handled in init, as I did see there is a reference to some sort of lock there.

[wenzeslaus]

binder link

[wenzeslaus]

I made further changes to the PR. Overall, I modeled the context manger to look like the open function in Python, so the original init is now like open. It seems to me like a good idea to follow an example of a standard Python function and it keeps the focus on the init function which keeps the overall extend of changes limited.

I replaced the previous changes with a separate class.

I kept the class, but it is simpler, more limited in what it can be used for. Specifically, it does not have open nor an init function which would start the session.

This way the old init function is kept exactly the same, and backwards compatibility is preserved.

No need to keep it exactly the same. The warning for changing return value was there exactly for this purpose. Now, the new code actually keep the standalone init function in the center of the interface.

The class, SessionHandle simply calls init() on SessionHandle.open(), and finish on SessionHandle.close(). Those functions are automatically called on use as a context manager.

The new approach relies on the standalone init to the "opening", so the init function of the handle and the enter functions do very little work. There is no open function anymore.

I changed the open-close naming to init-active-finish which just keeps whatever is already there, but I think some improvements would be useful there.

Both SessionHandle.open() and SessionHandle.__enter__() return gisrc to keep it as similar to init as possible, but perhaps that could be removed.

No need to be similar to init, so I tried to be similar to open, so enter returns self, so with init(...) as session is now possible.

And finally, I considered adding a boolean opened as some sort of "lock", to avoid calling session.open() on an already-opened/unclosed session, but I wasn't sure what the way to deal with that would be.

That seemed good. The enter now checks that, but there is no open which needs to check it anymore.

Perhaps some sort of exception, or ignorign it and writing to stderr.

Using with on finished/closed session raises a ValueError now.

Or perhaps that would be handled in init, as I did see there is a reference to some sort of lock there.

That's actually much bigger question. Maybe it is best not to address that in this PR.

@albertoparadisllop I'm sorry I was not able to continue in this in October.

[wenzeslaus]

Here are some possible usages. All assume something like:

sys.path.append(
    subprocess.check_output(["grass", "--config", "python_path"], text=True).strip()
)

import grass.script as gs
import grass.script.setup

Good cases

Explicit init-finish

session = gs.setup.init("~/grassdata/nc_basic_spm_grass7/user1")
gs.run_command("g.region", flags="p")
session.finish()

The old way still works:

gs.setup.init("~/grassdata/nc_basic_spm_grass7/user1")
gs.run_command("g.region", flags="p")
gs.setup.finish()

Context manager

Context manager:

with gs.setup.init("~/grassdata/nc_basic_spm_grass7/user1"):
    gs.run_command("g.region", flags="p")

The above shows that the newly introduced class is not intrusive and can be quite hidden. However, you can use the context manager and keep the value (this might be useful later when the class is more powerful):

with gs.setup.init("~/grassdata/nc_basic_spm_grass7/user1") as session:
    print(session)
    gs.run_command("g.region", flags="p")

Separate creation and context manage use like with Python's open. This is probably not slightly dangerous, so not great, but flexible. (Needed to allow for this to work also the old way, i.e., without context manager like Python's open does.)

session = gs.setup.init("~/nc_basic_spm_grass7/user1")
with session:
    print(session)
    gs.run_command("g.region", flags="p")

Bad cases

No cleaning happens

You forget to call finish or use the with statement.

gs.setup.init("~/grassdata/nc_basic_spm_grass7/user1")
gs.run_command("g.region", flags="p")

Similarly to the Python's open, the session is not finished. This could be prevented by creating an instance of the class and calling gs.setup.init() only in the enter method as in original proposal, but it would additionally require to also not provide an open method, so that only the with statement can be used to initialize a session (or an explicit session.__enter__() call which is possible, but clearly only for special cases). In that case, I think that not having an open method would be needed to discourage wrong usage. If there is an open method (to avoid need for the session.__enter__() call in some cases), I don't see much different in having the open method and making the session object construction and session initialization one step (as my current proposal does).

Raises exception

Use twice with with

session = gs.setup.init("~/nc_basic_spm_grass7/user1")
with session:
    print(session)
    gs.run_command("g.region", flags="p")

# raises ValueError
with session:
    print(session)
    gs.run_command("g.region", flags="p")

...raises an exception, so it behaves just like file1 = open("abc.txt", mode="w")...with...with.

Calling finish twice

Calling finish on session twice is not allowed:

session = gs.setup.init("~/grassdata/nc_basic_spm_grass7/user1")
gs.run_command("g.region", flags="p")
session.finish()
# raises ValueError
session.finish()

Alternatively, additional calls of the finish method can be ignored instead of raising an exception. Calling the standalone function finish fails, while the session handle for grass.jupyter.init allows calling finish multiple times to make it easy to run cell twice etc. Notably, Python's open allows calling close on the result multiple times, too (f = open("/tmp/test.txt", "w"); f.close(); f.close();).

Ends badly

Creating multiple sessions results in unexpected situation. One overwrites the other and calling finish on these objects is the same as calling finish on the same object.

session1 = gs.setup.init("~/grassdata/nc_basic_spm_grass7/user1")
session2 = gs.setup.init("~/grassdata/nc_basic_spm_grass7/user1")
session3 = gs.setup.init("~/grassdata/nc_basic_spm_grass7/PERMANENT")

print(session1, session2, session3)

gs.run_command("g.mapset", flags="p")

session3.finish()
session2.finish()
session1.finish()

This is again the same as when init is used with plain standalone finish (gs.setup.finish()).

Let me know if you see more issues or potential usages.

[wenzeslaus]

This PR now focuses on implementing the context manger to automatically close the session as asked for in #1848. However, it might be useful to compare it with #1834 focused on solving the issue for Jupyter notebooks.

#1834 takes a different approach focusing on usage in notebooks where context mangers are not useful for a session active through the whole notebook. The context manager is replaced in #1834 by finalizer approach (weakref.finalize), so cleaning happens automatically but in somewhat hidden fashion (when the object is garbage collected).

Another difference is that #1834 uses one global object (one reference kept as a global attribute) which is sometimes recycled for subsequent grass.jupyter.setup.init calls (much more common in notebooks). The global object is needed to ensure that there is always a reference to the session otherwise the session can be finished anytime (when the object is garbage collected). The full explanation is in 45c1e7f.

[wenzeslaus]

This is now ready for review and it includes latest updates from main for better testing and comparison. Here is a binder link.

@ninsbl Since you originally suggested this (#1848), can you please check this out?

@albertoparadisllop Feel free to comment.

[wenzeslaus]

When I remove myself from review, it shows my old review, so I'm leaving this comment to make my review neutral.

Edit: Well, that didn't work, but at the bottom, there is a dismiss review button which turns it into a neutral review (review comment).

Authored much of the current code.

[ninsbl]

Now I tested the binder link. This looks very good to me. Very elegant. Great work.
From my point of view, the only thing missing to fully replace the functionality of the external module grass_session is the ability to create new locations and mapsets in the init function. But that can be a next step...

[wenzeslaus]

Now I tested the binder link. This looks very good to me. Very elegant. Great work.

Thanks!

...the only thing missing...is the ability to create new locations and mapsets in the init function. But that can be a next step...

Definitively desired and a different PR. Feel free to open an issue/PR. For now, a good workaround is to use the grass executable through the subprocess package (the same works well for R too) or do some hacks to access the grass.script.create_location function (see e.g. test added in this PR).