obs_base don't create calib repo if it does not exist #25
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timj
reviewed
Mar 17, 2017
python/lsst/obs/base/cameraMapper.py
Outdated
| else: | ||
| if dafPersist.Storage.exists(uri=calibRoot): | ||
| calibStorage = dafPersist.Storage.makeFromURI(uri=calibRoot) | ||
| if not calibStorage: |
There was a problem hiding this comment.
do you mean if calibStorage is not None: here?
There was a problem hiding this comment.
I agree that the negation is slightly confusing here. It would be better to test explicitly against None.
timj
approved these changes
Mar 17, 2017
The problem is that "old" butler init API is translated to "new" butler API to create an output repository with mode = 'rw'. The result is that butler will create the storage (directory, in the case of a posix repository) if the directory does not exist. CameraMapper has both a 'root' repo and a 'calib' repo, so there are 2 storages associated with it. This was causing a race condition in obs_subaru: tests in multiple files are using the mapper with root=".", and they are only reading (I hope) from the mapper. But for these tests the calib repo does not exist on disk and is not required for the test. But HscMapper (CameraMapper subclass) will init CameraMapper with calibRoot='CALIB' if no other calibRoot is specified. So CameraMapper was causing Storage to create the calib storage folder, and when multiple tests were running this exposed a race condition where two tests could be calling makedirs at the same time. This fix causes CameraMapper to not create a CALIB folder if the storage does not exist, which is fine: CameraMapper does not write to the CALIB folder. The totally correct fix would be for CameraMapper subclasses to not pass in a calibRoot if they don't expect it to exist. Also, in daf_persistence, makedirs was changed to safeMakeDir, which would prevent the crash but would allow the tests to leave CALIB folders sprinkled around which would not be good.
n8pease
force-pushed
the
tickets/DM-9848
branch
from
5b7ef7c
to
e30d6a6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.