TemporaryDirectoryChanger frustrations and recommendations

[drewj-tp]

I want to rely on the TemporaryDirectoryChanger for testing because it's a great encapsulation for

• creating a temporary directory,
• entering that temporary directory,
• doing stuff,
• returning to the previous working directory,
• removing the temporary directory

But I frequently hit some snags that make it a frustrating experience. This is my attempt to document them, get some more guidance, and maybe talk about how to improve the functionality.

All the commands below are in a fresh ipython environment, no configured application, and with armi e38fa1a

My expectations / debugging process

I expect that I can use the class like

>>> from armi.utils.directoryChangers import TemporaryDirectoryChanger

>>> with TemporaryDirectoryChanger() as td:
...     pass
...

and directory changer will do its magic. There are no required arguments to the class so I'm led to believe this should work.

But this fails with

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[2], line 1
----> 1 with TemporaryDirectoryChanger() as td:
      2     pass

File ~\codes\armi\armi\utils\directoryChangers.py:282, in TemporaryDirectoryChanger.__init__(self, root, filesToMove, filesToRetrieve, dumpOnException, outputPath)
    280     # ARMIs temp dirs are in an context.APP_DATA directory: validate this is a temp dir.
    281     if pathlib.Path(context.APP_DATA) not in pathlib.Path(root).parents:
--> 282         raise ValueError(
    283             "Temporary directory not in a safe location for deletion."
    284         )
    286 # make the tmp dir, if necessary
    287 if not os.path.exists(root):

ValueError: Temporary directory not in a safe location for deletion.

Which is immediately odd. If the destruction of the temporary directory is also trying to remove the parent directory, that's definitely bad. But if the directory changer is managing the creation of a temporary directory, I expect that it should always be able to delete it.

(aside) After looking at the code base, you can infer that root in this case is my present working directory if the fast path is not activated. Definitely don't want to delete that. But the error doesn't tell me what root is here, so I usually assume it made a temporary directory that was not safe to delete. Which, huh?

Next step usually involves providing some paths. What about my current working directory.

>>> import pathlib
>>> cwd = str(pathlib.Path.cwd())
>>> cwd
'C:\\Users\\anjohnson\\codes\\armi'
>>> with TemporaryDirectoryChanger(cwd) as td:
...     pass
...
---------------------------------------------------------------------------
Exception                                 Traceback (most recent call last)
Cell In[9], line 1
----> 1 with TemporaryDirectoryChanger(cwd) as td:
      2     pass

File ~\codes\armi\armi\utils\directoryChangers.py:317, in TemporaryDirectoryChanger.__exit__(self, exc_type, exc_value, traceback)
    315 DirectoryChanger.__exit__(self, exc_type, exc_value, traceback)
    316 try:
--> 317     pathTools.cleanPath(self.destination, context.MPI_RANK)
    318 except PermissionError:
    319     if os.name == "nt":

File ~\codes\armi\armi\utils\pathTools.py:225, in cleanPath(path, mpiRank)
    222     valid = True
    224 if not valid:
--> 225     raise Exception(
    226         "You tried to delete {0}, but it does not seem safe to do so.".format(path)
    227     )
    229 # delete the file/directory from only one process
    230 if mpiRank == context.MPI_RANK:

Exception: You tried to delete C:\Users\anjohnson\codes\armi\temp-wkzeRrM5D2, but it does not seem safe to do so.

But, why? If it's trying to remove C:\Users\anjohnson\codes\armi, yeah definitely don't do that. But temp-wkzeRrM5D2 should be fair game for deletion.

Okay what about the fast path?

>>> from armi import context
>>> with TemporaryDirectoryChanger(context.getFastPath()) as td:
...     pass
...
---------------------------------------------------------------------------
Exception                                 Traceback (most recent call last)
Cell In[4], line 1
----> 1 with TemporaryDirectoryChanger(context.getFastPath()) as td:
      2     pass

File ~\codes\armi\armi\utils\directoryChangers.py:317, in TemporaryDirectoryChanger.__exit__(self, exc_type, exc_value, traceback)
    315 DirectoryChanger.__exit__(self, exc_type, exc_value, traceback)
    316 try:
--> 317     pathTools.cleanPath(self.destination, context.MPI_RANK)
    318 except PermissionError:
    319     if os.name == "nt":

File ~\codes\armi\armi\utils\pathTools.py:225, in cleanPath(path, mpiRank)
    222     valid = True
    224 if not valid:
--> 225     raise Exception(
    226         "You tried to delete {0}, but it does not seem safe to do so.".format(path)
    227     )
    229 # delete the file/directory from only one process
    230 if mpiRank == context.MPI_RANK:

Exception: You tried to delete C:\Users\anjohnson\codes\armi\temp-b7yYigWnFV, but it does not seem safe to do so.

Code base indicates the initial fast path is just your current directory

armi/armi/context.py

Line 139 in e38fa1a

_FAST_PATH = os.path.join(os.getcwd())

until someone activates it. Usually the Operator

armi/armi/operators/operator.py

Lines 257 to 272 in e38fa1a

	def _initFastPath():
	"""
	Create the FAST_PATH directory for fast local operations.
	Notes
	-----
	The FAST_PATH was once created at import-time in order to support modules
	that use FAST_PATH without operators (e.g. Database). However, we decided
	to leave FAST_PATH as the CWD in INTERACTIVE mode, so this should not
	be a problem anymore, and we can safely move FAST_PATH creation
	back into the Operator.
	If the operator is being used interactively (e.g. at a prompt) we will still
	use a temporary local fast path (in case the user is working on a slow network path).
	"""
	context.activateLocalFastPath()

>>> context.activateLocalFastPath()

>>> context.getFastPath()
'C:\\Users\\anjohnson\\AppData\\Roaming\\armi\\0-20250604100736209282'

>>> with TemporaryDirectoryChanger(context.getFastPath()) as td:
...     pass
...

That one works.

Working use cases

It seems the most reliable way to use the temporary directory change is to pass the activated fast path. If that is correct and the intended workflow, I would recommend we add that to the TemporaryDirectoryChanger docstring.

But then I look at ARMI's testing, and I see calls like

armi/armi/nuclearDataIO/cccc/tests/test_fixsrc.py

Line 36 in e38fa1a

with TemporaryDirectoryChanger() as newDir:

Where the temp dir is created without arguments. And, without arguments, it uses the fast path

armi/armi/utils/directoryChangers.py

Lines 278 to 279 in e38fa1a

	if root is None:
	root = context.getFastPath()

I presume this works because the fast path is configured at the start of testing

armi/armi/conftest.py

Lines 63 to 69 in e38fa1a

	# set and create a test-specific FAST_PATH for parallel unit testing
	# Not all unit tests have operators, and operators are usually
	# responsible for making FAST_PATH, so we make it here.
	# It will be deleted by the atexit hook.
	context.activateLocalFastPath()
	if not os.path.exists(context.getFastPath()):
	os.makedirs(context.getFastPath())

This would be good guidance to include in the temporary directory changer docstring.

Recommendations

These recommendations are predicated on my understanding of what ARMI is doing, and if what I'm trying to do is correct. I can make a PR with these recommendations, but I want to make sure they're correct.

1. Document the relationship between root and context.getFastPath in the constructor for the TemporaryDirectoryChanger
2. Document these file system related errors in TemporaryDirectoryChanger.__exit__ in a Raises section to provide guidance on why these errors may be raised and how to mitigate them.
3. Provide a better error message if the pathTools.cleanPath call errors to explain why the error and/or how to get around the error. Exception: You tried to delete C:\Users\anjohnson\codes\armi\temp-b7yYigWnFV, but it does not seem safe to do so. doesn't help with mitigation. Especially when I'm trying to delete that temp directory and it should be safe to do so. Why is it not safe?
4. Put the directory name in the ValueError: Temporary directory not in a safe location for deletion. message to aid in debugging.

Extensions

I think it's reasonable to expect TemporaryDirectoryChanger() to work always, even if the fast path is not activated. Python has the tempfile module for making temporary files and directories.

I think it's reasonable to expect TemporaryDirectoryChanger(cwd) to work always. I have asked to make a temporary directory in my current working directory. That temporary directory should be fair game for deletion. Same for any base directory that I have the ability to make a directory and write files.

I think it's reasonable for the TemporaryDirectoryChanger, upon failing to remove the temporary directory, to log a warning rather than raise a fatal error. So long as we've returned the caller to their original working directory, does it really matter ARMI did not remove the temporary files? If we runLog.warning to alert to the un-removed temporary directory, the user can still clean them up after the fact. Alternatively, it can document what type of exception (other than a base Exception) it would raise and allow clients to try/except around this behavior.

[john-science]

I expect that I can use the class like

>>> from armi.utils.directoryChangers import TemporaryDirectoryChanger

>>> with TemporaryDirectoryChanger() as td:
...     pass
...

@drewj-tp The TemporaryDirectoryChanger is not meant to work outside unit tests. The above example code you posted should definitely. If you want it to run, you could mock up a version of our unit testing environment:

import armi
armi.configure(armi.apps.App())

from armi.conftest import bootstrapArmiTestEnv
bootstrapArmiTestEnv()

from armi.utils.directoryChangers import TemporaryDirectoryChanger

with TemporaryDirectoryChanger() as td:
    pass

Or, obviously, you could just copy/paste your code directly into a unit test. As you pointed out, there are hundreds of examples of that code you wrote working exactly as-is inside a unit test:

armi/armi/cases/tests/test_cases.py

Line 91 in e38fa1a

with directoryChangers.TemporaryDirectoryChanger():

armi/armi/cases/tests/test_cases.py

Lines 122 to 123 in e38fa1a

	def test_startCoverage(self):
	with directoryChangers.TemporaryDirectoryChanger():

armi/armi/cases/tests/test_cases.py

Lines 165 to 166 in e38fa1a

	def test_endProfiling(self):
	with directoryChangers.TemporaryDirectoryChanger():

So, you started your statement by saying:

I want to rely on the TemporaryDirectoryChanger for testing

I posit that if you only ever use the TemporaryDirectoryChanger inside an ARMI-based unit test, you'll be fine.

[john-science]

More conversation is needed. This is one of those ARMI features that exists even though the Python standard library provides a better tool, e.g.:

import tempfile
with tempfile.TemporaryDirectory() as tmp_dir:
    pass

Also, pytest has a well-loved temporary directory tool.

Now, MAYBE you can argue that when an ARMI unit test fails we can retrieve the temp dir easily, right in the ARMI repo area. And maybe that's useful enough to keep this tool in ARMI.

But my guess is that this ARMI tool only exists because:

a. Python's standard library tempfile wasn't introduced until Python 2.6, and this ARMI feature predates that. OR...
b. Whoever create TemporaryDirectoryChanger didn't know this was already in the standard library.

Though, perhaps there is a third option. MpiDirectoryChanger exists and might be the reason ARMI has all this tooling. That is only used in one place anywhere in the world (in a project you own):

armi/armi/utils/directoryChangersMpi.py

Line 26 in 14392c8

class MpiDirectoryChanger(directoryChangers.DirectoryChanger):

I would be happy to add some documentation to the tooling, of course. For sure.

[drewj-tp]

If the temp dir changer was removed from armi "production" API, I would re-implement it using tempfile to make the dir and then a DirectoryChanger to go into it.

This is one of those ARMI features that exists even though the Python standard library provides a better tool

I could see the temp directory changer linking nicely with tempfile. That would remove the need to handle the naming of the temp directory via random strings

armi/armi/utils/directoryChangers.py

Lines 282 to 287 in 14392c8

	@classmethod
	def GetRandomDirectory(cls, root):
	return os.path.join(
	root,
	"temp-" + "".join(random.choice(string.ascii_letters + string.digits) for _ in range(10)),
	)

That's exactly what tempfile is for 😅

Also, pytest has a well-loved temporary directory tool.

That thing rocks. If we're just looking to run a test in a temp dir, that's definitely what we should build on. It plays nice w/ parallel testing too. But I would not consider it viable for usage outside of testing. I don't want to make pytest a runtime dependency

[john-science]

I don't want to make pytest a runtime dependency

Agreed.

I think trying a quick re-write of TemporaryDirectoryChanger where it becomes a simple pass-through for tempfile is probably the best direction for ARMI.

All this being said, if you want to use TemporaryDirectoryChanger downstream at runtime, not just in tests, I would seriously consider just using tempfile directly. It's dependable enough to be in the standard library. Just IMO.

[opotowsky]

Poking my head in to say that yes, TempDirChanger is used downstream at runtime.

The use of this code is convoluted and not well documented, so at a minimum we should address that.

Within ARMI it isn't much of an issue, but downstream (within unit testing) it's a mess, and I think that's because folks just try a few things until they find something that works, rather than using a well documented bullet proof workflow.

P.S. I would LOVE to change most uses of it to be pythons tempfile. It would be a lot more robust.

[john-science]

I am not sure there is a good reason for ARMI to support a Temp Dir feature. Since this feature was added to ARMI, it has since been bettered by the Python standard library, and then again by PyTest.

Now, people would be upset if the feature was deleted from ARMI. So what does that leave as the best option?

Perhaps the ARMI Temp Dir feature could be rewritten as a light wrapper around the Python standard library version. I could preserve the ARMI API for now, but remove the need to continue developing on this ARMI feature. And any other features the standard library has would be available.

Just thinking aloud. Obviously.