Skip to content

Set GMT_SESSION_NAME to a unique name on Windows for multiprocessing support #2938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
seisman merged 4 commits into from
Jan 2, 2024
Merged

Set GMT_SESSION_NAME to a unique name on Windows for multiprocessing support #2938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
12383e1
Add a test to make sure the workaround for multiprocessing support works
seisman Jan 1, 2024
07ae8f1
Set GMT_SESSION_NAME to a unique value so multiprocessing works on Wi…
seisman Jan 1, 2024
236f874
Move imports to top
seisman Jan 1, 2024
04646ad
Merge branch 'main' into windows-multiprocessing
seisman Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions pygmt/session_management.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
"""
Modern mode session management modules.
"""
import os
import sys

from pygmt.clib import Session
from pygmt.helpers import unique_name


def begin():
Expand All @@ -12,6 +16,10 @@ def begin():

Only meant to be used once for creating the global session.
"""
# On Windows, need to set GMT_SESSION_NAME to a unique value
if sys.platform == "win32":
os.environ["GMT_SESSION_NAME"] = unique_name()
Comment on lines +19 to +21
Copy link
Member

@weiji14 weiji14 Jan 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I thought this should best be handled upstream in GMT, but then I saw the note from https://github.com/GenericMappingTools/gmt/blob/104bcdbafd13eb96260b445910ce3587a362dbc0/src/gmt_api.c#L1050-L1057:

	/* OK, the trouble is the following. On Win, if the Windows executables are run from within a bash window
	   gmtapi_get_ppid returns different values for each call, and this completely breaks the idea
	   of using the constant PPID (parent PID) to create unique file names for each session.
	   So, given that we didn't yet find a way to make this work from within MSYS (and likely Cygwin)
	   we are forcing PPID = 0 in all Windows variants unless set via GMT_SESSION_NAME. A corollary of this
	   is that Windows users running many bash windows concurrently should use GMT_SESSION_NAME in their scripts
	   to give unique values to different scripts.  */

Would setting a unique GMT_SESSION_NAME here in PyGMT result in any issues then? I couldn't run git blame on those lines to find out more context.

Copy link
Member Author

@seisman seisman Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This note (https://docs.generic-mapping-tools.org/dev/begin.html#note-on-unix-shells) is also useful. So, the key point for how GMT modern mode works is having a unique and invariable value so that GMT modules know where to find the session directory and the historical information. The value can be either the constant PPID or a number manually set by GMT_SESSION_NAME.

Would setting a unique GMT_SESSION_NAME here in PyGMT result in any issues then?

All tests still pass, so I believe it has no side effects.

Copy link
Member

@weiji14 weiji14 Jan 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but maybe we should check if GMT_SESSION_NAME is set by the user first before overriding it. Something like this maybe (untested):

Suggested change
# On Windows, need to set GMT_SESSION_NAME to a unique value
if sys.platform == "win32":
os.environ["GMT_SESSION_NAME"] = unique_name()
# On Windows, need to set GMT_SESSION_NAME to a unique value
if sys.platform == "win32" and not "GMT_SESSION_NAME" in os.environ:
os.environ["GMT_SESSION_NAME"] = unique_name()

Copy link
Member Author

@seisman seisman Jan 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users should NEVER set GMT_SESSION_NAME as a global environment variable. Even if they set GMT_SESSION_NAME, we should override it in PyGMT, otherwise multiprocessing won't work.

Here are steps to understand why GMT_SESSION_NAME should be overridden. Open a terminal, define the variable:

export GMT_SESSION_NAME=123456

and run gmt begin. Then you'll see the session directory ~/.gmt/sessions/gmt_session.123456/; all information will be saved in this directory. So, if a global GMT_SESSION_NAME is set, processes will write information and output to the same directory, leading to crashes.


prefix = "pygmt-session"
with Session() as lib:
lib.call_module(module="begin", args=prefix)
Expand Down
29 changes: 29 additions & 0 deletions pygmt/tests/test_session_management.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
"""
Test the session management modules.
"""
import multiprocessing as mp
import os
from importlib import reload
from pathlib import Path

import pytest
from pygmt.clib import Session
Expand Down Expand Up @@ -57,3 +60,29 @@ def test_gmt_compat_6_is_applied(capsys):
# Make sure no global "gmt.conf" in the current directory
assert not os.path.exists("gmt.conf")
begin() # Restart the global session


def _gmt_func_wrapper(figname):
"""
A wrapper for running PyGMT scripts with multiprocessing.

Currently, we have to import pygmt and reload it in each process. Workaround from
https://github.com/GenericMappingTools/pygmt/issues/217#issuecomment-754774875.
"""
import pygmt

reload(pygmt)
fig = pygmt.Figure()
fig.basemap(region=[10, 70, -3, 8], projection="X8c/6c", frame="afg")
fig.savefig(figname)


def test_session_multiprocessing():
"""
Make sure that multiprocessing is supported if pygmt is re-imported.
"""
prefix = "test_session_multiprocessing"
with mp.Pool(2) as p:
p.map(_gmt_func_wrapper, [f"{prefix}-1.png", f"{prefix}-2.png"])
Path(f"{prefix}-1.png").unlink()
Path(f"{prefix}-2.png").unlink()
Comment on lines +86 to +88
Copy link
Member

@weiji14 weiji14 Jan 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably use GMTTempFile or a try-except block to remove the files automatically, even if the multiprocessing function fails.

Copy link
Member Author

@seisman seisman Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely not necessary here. If anything wrong happens, the two PNG files won't be generated, and Path(...).unlink() will raise an exception.