desihub / desisurvey

Code for desi survey planning and implementation
BSD 3-Clause "New" or "Revised" License
2 stars 7 forks source link

unit tests failing on NERSC #132

Closed sbailey closed 3 years ago

sbailey commented 3 years ago

desisurvey and surveysim unit tests broke at NERSC starting last night due to inability to read the tile table:

import desisurvey.tiles
desisurvey.tiles.Tiles().read_tiles_table()

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-1ff4b260b411> in <module>
----> 1 desisurvey.tiles.Tiles().read_tiles_table()

/global/common/software/desi/cori/desiconda/20200801-1.4.0-spec/code/desisurvey/master/py/desisurvey/tiles.py in __init__(self, tiles_file)
     54         # Read the specified tiles file.
     55         self.tiles_file = tiles_file or config.tiles_file()
---> 56         self.tiles_file = find_tile_file(self.tiles_file)
     57 
     58         tiles = self.read_tiles_table()

/global/common/software/desi/cori/desiconda/20200801-1.4.0-spec/code/desisurvey/master/py/desisurvey/tiles.py in find_tile_file(filename)
    366     dmname = desimodel.io.findfile(os.path.join('footprint', filename))
    367     dsdirname = os.environ.get('DESISURVEY_OUTPUT', None)
--> 368     dsname = os.path.join(dsdirname, filename)
    369     localname = filename
    370     namedict = dict(DESISURVEY=(dsname, os.path.exists(dsname)),

/global/common/software/desi/cori/desiconda/20200801-1.4.0-spec/conda/lib/python3.8/posixpath.py in join(a, *p)
     74     will be discarded.  An empty last part will result in a path that
     75     ends with a separator."""
---> 76     a = os.fspath(a)
     77     sep = _get_sep(a)
     78     path = a

TypeError: expected str, bytes or os.PathLike object, not NoneType

Previously $DESISURVEY_OUTPUT was not a required environment variable, but it looks like now it is again. What should this be set to at NERSC for a general DESI user? I'm nervous about use cases for reading the tiles file as input in a directory called "output". Can we preserve the ability for regular users to access the default surveyops survey tiles file without causing them to try writing into that same directory?

@schlafly @dkirkby

schlafly commented 3 years ago

Thanks, sorry, yes---this is on me. I ran the tests, but yes, I had DESISURVEY_OUTPUT set. I'll take a look.

schlafly commented 3 years ago

This is easy to fix for people who only want to read the tile file. I'll push a fix in a bit.

For background: some of things we used to treat as output (priorities, HA optimizations) are now optionally part of the tile file. So the hour angle optimization writes out an updated tile file to DESISURVEY_OUTPUT. The intention was to fall back to the desimodel tile file if this was unavailable, though there was a dumb bug requiring DESISURVEY_OUTPUT to be set for that check to fail. We'll still need DESISURVEY_OUTPUT to be set for real uses of the simulations, since they require ephemerides and HA optimization files in DESISURVEY_OUTPUT, which has always been the case.

schlafly commented 3 years ago

Should be resolved in https://github.com/desihub/desisurvey/commit/a377d6c461190ea319c4033a1e3a6690649c2eae. At least, it passes the tests for me, with DESISURVEY_OUTPUT no longer set. Okay to close?

sbailey commented 3 years ago

Thanks for the quick fix. I confirm that it works at NERSC and I have installed the master checkout. Closing.