Skip to content
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.

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

Add default output directory #462

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ElliottKasoar
Copy link
Member

Changes default file_prefix to include ./janus_results/ (originally results, but this clashes with mace training outputs, meaning that output files will normally all be contained within this directory, as discussed in #359.

I initially thought we may want a dedicated option for the file directory, but since file_prefix directories are created anyway, something like --file-prefix ./alt_dir/prefix, or --file-prefix ./just-prefix (to avoid creating a new directory), which both already work, is just as effective.

One slight question mark on the dev side of things is currently this will create the janus_results directory if it doesn't exist, including when running tests, but I only clean up the files, not the directory itself, so running the tests may leave behind a new empty directory. I thought this was probably preferable to a lot of extra logic figuring out if it starts empty and so is safe to delete/potential test failures if it's not.

@ElliottKasoar ElliottKasoar added the enhancement New/improved feature or request label Mar 5, 2025
@ElliottKasoar ElliottKasoar self-assigned this Mar 5, 2025
@ElliottKasoar ElliottKasoar changed the title Add output dir Add default output directory Mar 5, 2025
@@ -26,10 +26,10 @@ def test_help():

def test_eos():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just use pytest's tmp_path factory fixture?

Copy link
Member Author

@ElliottKasoar ElliottKasoar Mar 5, 2025

Choose a reason for hiding this comment

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

We do generally, but that doesn't test that the default file_prefix creates the files we expect. (Unless we did something like change directories into a temporary directory, and ran everything from there)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could monkeypatch to check the directory it would create and then over-write the return to tmp_path

Copy link
Member Author

@ElliottKasoar ElliottKasoar Mar 5, 2025

Choose a reason for hiding this comment

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

We probably could, but the exact way the filenames are built varies a little, so I'm not sure how easy it would be in a completely general way, and I think we might risk missing unusual interactions.

Arguably the current form is more of an integration test, since we test the file builder separately anyway, which I think is practically what we'd be checking again if we don't write to the actual default location.

Copy link
Collaborator

@oerc0122 oerc0122 Mar 5, 2025

Choose a reason for hiding this comment

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

Yeah. That was one of my other thoughts. Leaving behind miscellaneous files (even empty folders) in the test suite just feels unpleasant. I think at the very least we want to delete them before attempting the test.

Co-authored-by: Jacob Wilkins <46597752+oerc0122@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New/improved feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants