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
DM-26774: Instrument-finding code incorrectly requires a data query #145
Conversation
97c242c
to
a6bf8b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks okay. Can we add a simple test to tests/test_graphBuilder.py with an empty query? (also, HelperTestCase is a bit too generic a unittest TestCase class name for my liking).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but may be worth replacing tempfile.TemporaryDirectory
, check my comments.
@@ -153,7 +180,21 @@ def registerDatasetTypes(registry, pipeline): | |||
registry.registerDatasetType(datasetType) | |||
|
|||
|
|||
def makeSimpleQGraph(nQuanta=5, pipeline=None, butler=None, root=None, skipExisting=False, inMemory=True): | |||
def makeSimplePipeline(nQuanta, instrument=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstring would be nice.
|
||
|
||
def makeSimpleQGraph(nQuanta=5, pipeline=None, butler=None, root=None, skipExisting=False, inMemory=True, | ||
userQuery=""): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add docstring for userQuery
?
tests/test_graphBuilder.py
Outdated
def testDefault(self): | ||
"""Simple test to verify makeSimpleQGraph can be used to make a Quantum | ||
Graph.""" | ||
with tempfile.TemporaryDirectory() as root: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tim pointed out to me that tempfile.TemporaryDirectory
may not behaving well if there are errors during deleting the directory, it does not specify ignore_errors=True
when it calls shutil.rmtree
. I had to implement trivial context manager to workaround that: https://github.com/lsst/ctrl_mpexec/blob/master/tests/test_cmdLineFwk.py#L78-L87. I think you need something like that too (better of course is to have it somewhere in the common code base but I don't know where that could be). Same for the method below.
5a70acb
to
b1b024e
Compare
on branch tickets/DM-26774-transfer
on branch tickets/DM-26774-transfer
on branch tickets/DM-26774-transfer
b1b024e
to
d223dfa
Compare
No description provided.