DM-13835: Cannot ingest empty data #33
Conversation
Having a parser quit the program makes sense when the user provides bad arguments to the main program, but is dangerous when it happens as an implementation detail of a submodule.
| Raises | ||
| ------ | ||
| RuntimeError | ||
| Raised if there are no files to ingest. |
There was a problem hiding this comment.
Thanks for documenting this. We need more docstrings that call out known exceptions.
python/lsst/ap/verify/ingestion.py
Outdated
| """ | ||
| if not dataFiles: | ||
| raise RuntimeError("No raw files to ingest.") |
There was a problem hiding this comment.
Maybe be a little more explicit here, to help with debugging: "No raw files to ingest; dataFiles is %s."%dataFiles
There was a problem hiding this comment.
The if statement guarantees that won't display anything interesting.
There was a problem hiding this comment.
dataFiles could be None, False, [], (), {}, ''. It might be informative to know which of those it was.
There was a problem hiding this comment.
Ugh, weakly typed languages. 😞 I'll write it with %r, then.
python/lsst/ap/verify/ingestion.py
Outdated
| """ | ||
| if not calibDataFiles: | ||
| raise RuntimeError("No calib files to ingest.") |
There was a problem hiding this comment.
As above, please include calibDataFiles in the raised string.
python/lsst/ap/verify/ingestion.py
Outdated
| with tarfile.open(defectTarball, "r") as opened: | ||
| if opened.getNames(): | ||
| if not os.path.isdir(defectDir): | ||
| os.mkdir(defectDir) |
There was a problem hiding this comment.
Maybe use pathlib here in place of if exist: mkdir, to avoid os.mkdir race conditions on parallel ingest:
pathlib.Path(defectDir).mkdir(parents=True, exist_ok=True)
https://stackoverflow.com/a/14364249
https://docs.python.org/3/library/pathlib.html#pathlib.Path.mkdir
There was a problem hiding this comment.
I'm not sure parallel ingestion is possible, but your solution is more elegant.
| butler = self._rawButler() | ||
| for datum in IngestionTestSuite.rawData: | ||
| dataId = {'visit': datum['visit']} | ||
| self.assertEqual(butler.datasetExists('raw', dataId), datum['file'] not in badFiles) |
There was a problem hiding this comment.
Ooog. I'm not too keen on this syntax (it was hard for me to parse), but I'm not sure of a better approach given the butler methods that exist.
There was a problem hiding this comment.
I could do the approach of if in badFiles: self.assertFalse(...) else: self.assertTrue(...), but I'd find that ickier.
Attempts to ingest empty lists of files now raise an exception. Unit tests have been updated to reflect the new behavior.
Path.mkdir provides a clean postcondition on the directory's existence, unlike os.mkdir.
This factoring makes it easier to share the information among test cases.
kfindeisen
force-pushed
the
tickets/DM-13835
branch
from
1fa5d53
to
3af2236
Compare
This PR changes the behavior of
DatasetIngestTask's private methods from crashing to raising a specific exception. It also adds a unit test that could not be implemented because of this bug (at least, not while we were still usingtestdata_decam).