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 #2138. #2154

Merged
merged 5 commits into from Jan 3, 2024
Merged

Fix #2138. #2154

merged 5 commits into from Jan 3, 2024

Conversation

moustakas
Copy link
Member

Tentative fix for #2138 but we'll need to confirm if this fixes this part of the issue you identified, @sbailey:

@moustakas we're getting test_photo failures at NERSC, e.g. see https://data.desi.lbl.gov/desi/spectro/redux/dailytest/log/perlmutter/desispec.log . 
When run as part of the full test suite, I'm getting one failure like

...
        if legacysurveydir is None:
            legacysurveydir = os.path.join(desi_root, 'external', 'legacysurvey', 'dr9')
    
        if not os.path.isdir(legacysurveydir):
            errmsg = f'Legacy Surveys directory {legacysurveydir} not found.'
            log.critical(errmsg)
>           raise IOError(errmsg)
E           OSError: Legacy Surveys directory /tmp/tmpgio7la0q/external/legacysurvey/dr9 not found.
and two failures like

        fiberfile = os.path.join(fiberassign_dir, stileid[:3], f'fiberassign-{stileid}.fits.gz')
        if not os.path.isfile(fiberfile):
            fiberfile = fiberfile.replace('.gz', '')
            if not os.path.isfile(fiberfile):
                errmsg = f'Fiber assignment file {fiberfile} not found!'
                log.critical(errmsg)
>               raise IOError(errmsg)
E               OSError: Fiber assignment file /tmp/tmpgio7la0q/target/fiberassign/tiles/trunk/000/fiberassign-000019.fits not found!
It looks like some other test might be resetting $DESI_ROOT or $DESI_ROOT_READONLY out from under you. test_io does some $DESI_ROOT manipulation, but it also attempts to set it back when done.

The functions in desispec.io.photo as well as the test_photo suite relies entirely on desispec.io.meta.get_desi_root_readonly doing the right thing, so if another unit test is changing the top-level DESI path, then I'm going to need help tracking down who/what/where.

Copy link
Contributor

@sbailey sbailey left a comment

Choose a reason for hiding this comment

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

This is an improvement over hardcoding /global/cfs/cdirs/desi in the unit tests themselves, but I posted one inline comment about a bug, and I don't think this completely addresses the underlying issue from #2138. Will test more after the desi_root undefined issue is addressed.

[login36 desispec] pytest py/desispec/test/test_photo.py 
============================= test session starts ==============================
platform linux -- Python 3.10.8, pytest-7.2.0, pluggy-1.0.0
rootdir: /global/common/software/desi/users/sjbailey/desispec
plugins: astropy-header-0.1.2, filter-subpackage-0.1.1, openfiles-0.5.0, doctestplus-0.12.1, arraydiff-0.3, anyio-3.6.2, astropy-0.10.0, asdf-2.14.3, cov-4.0.0, remotedata-0.4.0, hypothesis-6.62.0, mock-3.10.0
collected 0 items / 1 error                                                    

==================================== ERRORS ====================================
_______________ ERROR collecting py/desispec/test/test_photo.py ________________
py/desispec/test/test_photo.py:12: in <module>
    if 'NERSC_HOST' in os.environ and os.getenv('DESI_SPECTRO_DATA') == os.path.join(desi_root, 'spectro', 'data'):
E   NameError: name 'desi_root' is not defined
=========================== short test summary info ============================
ERROR py/desispec/test/test_photo.py - NameError: name 'desi_root' is not defined
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=============================== 1 error in 4.85s ===============================

py/desispec/test/test_photo.py Outdated Show resolved Hide resolved
@moustakas
Copy link
Member Author

With the latest commit at NERSC I get

% pytest py/desispec/test/test_photo.py
======================================================================================= test session starts =======================================================================================
platform linux -- Python 3.10.8, pytest-7.2.0, pluggy-1.0.0
rootdir: /global/u2/i/ioannis/code/desihub/desispec
plugins: astropy-header-0.1.2, filter-subpackage-0.1.1, openfiles-0.5.0, doctestplus-0.12.1, arraydiff-0.3, anyio-3.6.2, astropy-0.10.0, asdf-2.14.3, cov-4.0.0, remotedata-0.4.0, hypothesis-6.62.0, mock-3.10.0
collected 4 items

py/desispec/test/test_photo.py ....                                                                                                                                                         [100%]

======================================================================================= 4 passed in 25.24s ========================================================================================

The only new / different assumption I had to make was

#surveyops_dir = os.environ['DESI_SURVEYOPS']
surveyops_dir = desi_root+'/survey/ops/surveyops/trunk' # assumes a standard installation / environment

Hopefully this is alright @sbailey.

@sbailey
Copy link
Contributor

sbailey commented Jan 3, 2024

Thanks. This PR fixes the second problem listed in #2138, i.e. the failure of test_photo.py when run by itself. Good. We still have the problem that some other test is changing DESI_ROOT and not resetting it, thus breaking test_photo.py when all of the tests are run together with a single pytest invocation. So I'll merge this PR now, but leave #2138 open until we can debug the other cross-test problem.

@sbailey sbailey merged commit dc2e306 into main Jan 3, 2024
24 checks passed
@sbailey sbailey deleted the fix-2138 branch January 3, 2024 05:15
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.

None yet

2 participants