Refactor work_dir creation

[ccarouge]

Fixes #150

Added a mkdir() wrapper to use throughout the code:

• benchcab to create src/, hence removing the setup_src_dir() function.
• fluxsite to create the task directories within setup_task()
• workdir to create the fluxsite run directory tree.

Defined FLUXSITE_DIRS: a dictionary in internal.py that contains all the individual directories for the fluxsite runs.

Change the creation and deletion of fluxsite directory tree and source directory to use relative paths instead of absolute paths.

[codecov]

Codecov Report

Merging #171 (f083d3e) into main (56448d4) will decrease coverage by 0.40%.
Report is 1 commits behind head on main.
The diff coverage is 95.83%.

@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
- Coverage   88.97%   88.58%   -0.40%     
==========================================
  Files          26       26              
  Lines        1660     1603      -57     
==========================================
- Hits         1477     1420      -57     
  Misses        183      183              

Files	Coverage Δ
benchcab/comparison.py	61.36% <ø> (ø)
benchcab/internal.py	90.69% <100.00%> (+0.22%)	⬆️
benchcab/utils/fs.py	100.00% <100.00%> (ø)
benchcab/workdir.py	100.00% <100.00%> (ø)
tests/test_benchcab.py	100.00% <ø> (ø)
tests/test_comparison.py	100.00% <100.00%> (ø)
tests/test_fluxsite.py	100.00% <100.00%> (ø)
tests/test_fs.py	100.00% <100.00%> (ø)
tests/test_workdir.py	100.00% <100.00%> (ø)
benchcab/fluxsite.py	83.13% <87.50%> (+0.09%)	⬆️
... and 1 more

[SeanBryan51]

Looks good, I just have a question/comment on the use of chdir

[SeanBryan51]

We can probably remove the chdir here? I notice you have used chdir elsewhere in the code. Was it required to make the tests pass?

Suggested change

	with chdir(self.root_dir):
	mkdir(internal.SRC_DIR, exist_ok=True, verbose=True)
	mkdir(internal.SRC_DIR, exist_ok=True, verbose=True)

[ccarouge]

In the current instantiation of the tests, without mkdir the paths will be created where the tests are run and not in the temporary location. The paths are deleted right away in the tests so nobody will see them but it is still an undesired side effect. Especially if something goes wrong and they are not deleted.

With the redesign of the tests, since we have a fixture that changes the CWD for the whole test, it will work without chdir.

I'm happy to wait for the tests redesign to go in first.

[SeanBryan51]

I'm surprised the files are not being created in the temporary location since there is a run_around_tests() autouse fixture which changes the CWD. But I'm happy for this to wait until #172 goes in.

[ccarouge]

I'll double check...

[SeanBryan51]

Doing this made me wonder if we shouldn't define a "FLUXSITE_DIR" dictionary in internal.py instead of creating the list when running benchcab... But it might wait for a redesign of internal.py instead.

Yep it is basically a constant so it could go in internal.py. We could define the relative path list in internal.py and iterate over that?

[ccarouge]

@SeanBryan51 I think we need to discuss our strategy to create/remove directories. For the moment, we are going towards using relative paths to create the fluxsite run tree. But the clean function uses absolute paths:

def clean_directory_tree(root_dir=internal.CWD):
    """Remove pre-existing directories in current working directory."""
    src_dir = Path(root_dir, internal.SRC_DIR)
    if src_dir.exists():
        shutil.rmtree(src_dir)

    run_dir = Path(root_dir, internal.RUN_DIR)
    if run_dir.exists():
        shutil.rmtree(run_dir)

This makes it harder to remember what they do, in particular when writing tests as root_dir becomes redundant in some cases but not others.

Since internal.py contains the relative paths SRC_DIR and RUN_DIR, should we instead write clean_directory_tree with relative paths and handle the location we call it from the outside? Or is it safer to have functions work on absolute paths so they work always the same way no matter how they are called?

[SeanBryan51]

Since internal.py contains the relative paths SRC_DIR and RUN_DIR, should we instead write clean_directory_tree with relative paths and handle the location we call it from the outside?

Yes. I think ideally we should use relative paths whenever paths are prefixed by the CWD. Maybe we can take a look at this when we fix issue #162?

[SeanBryan51]

Would a list be better here? I.e.

FLUXSITE_DIRS = [
    FLUXSITE_RUN_DIR,
    FLUXSITE_LOG_DIR,
    ...
]

I don't mind either way, but it seems using a list would have less changes in the code base.

[ccarouge]

I don't like having 2 places with the same constants: the constant with its name and then a list with all of them that is used only once in the code after that. Hence the idea of a dictionary so that references to these paths are always handled in the same way throughout the code.