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

Fix unit tests #2319

Merged
merged 6 commits into from
Aug 15, 2024
Merged

Fix unit tests #2319

merged 6 commits into from
Aug 15, 2024

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Aug 14, 2024

This PR

For the newly failing unit tests (#2315), the problem was triggered by assemble_fibermap in PR #2313 needing CalibFinder to look up which fibers need FIBERSTATUS bits set, but

  1. the unit tests were checking corner cases in very old data that didn't have a dark model in $DESI_SPECTRO_DARK, causing the CalibFinder creation to crash even though assemble_fibermap itself doesn't need a dark; this was fixed using fallback_on_dark_not_found=True. I considered updating CalibFinder itself to only crash if a missing dark was requested (instead of crashing at initialization time whether or not the dark was needed), but that was going to be a bigger refactor and fallback_on_dark_not_found=True within assemble_fibermap worked fine.
  2. test_calibfinder was changing $DESI_SPECTRO_CALIB but not resetting it when done, causing test_io_fibermap to fail due to a leftover bad $DESI_SPECTRO_CALIB value. Now fixed.

For bootcalib #2030, the unit tests now download from https://data.desi.lbl.gov/public/epo/example_files/spectest instead of https://portal.nersc.gov/project/desi/data/spectest which no longer exists. Full disclosure: previously the tests were run out of the desispec clone directory and the tests files were left behind in the top-level dir, and re-used if they already existed from a previous test run. Now the test is using a temp directory every time, but that means it also has to download the files every time. If the download fails it just skips the test (which is what is has been doing for over a year since the old URL disappeared anyway). When running at NERSC, the download is a fraction of a second, but we should keep an eye on this if it becomes problematically slow from elsewhere.

In general, I cleaned up the other tests so that they don't write test files into the desispec git clone directory. In addition to being good practice, this now enables tests to be run from a GPU compute node which mounts /global/common/software readonly. To minimize changes to the tests themselves, the basic model I followed was:

  • setUpClass, run before any tests, caches origdir=os.getcwd(), creates a temporary directory testdir=tempfile.mkdtemp(), and move there with os.chdir(testdir).
  • setUp, run at the start of every test, re-runs os.chdir(self.testdir) in case any previous test had moved out of that dir
  • tearDownClass, run at the end of all tests, removes any temporary files and goes back to the original os.chdir(origdir) so that the next test suite starts from a known location.

Some tests already had some usage of tempfile, e.g. to specify the full path of every file instead of os.chdir + writing files without a path. I left those as-is.

Summarizing, this now works to run tests from a GPU compute node that has mounted a git clone directory in read-only mode:

salloc -N 1 -C gpu -A desi_g --gpus-per-node=4 -t 04:00:00 -q interactive
cd /global/common/software/desi/users/sjbailey/desispec
pytest

Copy link
Member

@weaverba137 weaverba137 left a comment

Choose a reason for hiding this comment

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

Besides the comment about cleaning up temporary directories, this looks fine.

py/desispec/test/test_bootcalib.py Show resolved Hide resolved
py/desispec/test/test_calibfinder.py Show resolved Hide resolved
@sbailey
Copy link
Contributor Author

sbailey commented Aug 15, 2024

I have updated the tests to remove their temporary directories using the pattern

if cls.testdir.startswith(tempfile.gettempdir()) and os.path.exists(cls.testdir):
    shutil.rmtree(cls.testdir)

I think this is sufficient to ensure that we only remove directories that were created by tempfile.mkdtemp and won't accidentally remove $SCRATCH or $HOME etc.

@weaverba137 please re-review.

Copy link
Member

@weaverba137 weaverba137 left a comment

Choose a reason for hiding this comment

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

Looks good to merge!

@sbailey sbailey merged commit 1cbedbf into main Aug 15, 2024
26 checks passed
@sbailey sbailey deleted the fix_tests branch August 15, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

desispec calib unit tests failing Many bootcalib tests are skipped because of missing data
2 participants