DM-27667: Add pipetask option to execute graph subset #103
Conversation
andy-slac
force-pushed
the
tickets/DM-27667
branch
from
0be18b6
to
8858a70
Compare
| from lsst.daf.butler.core.utils import iterable | ||
|
|
||
|
|
||
| def _split_commas_int(context, param, values): |
There was a problem hiding this comment.
Why has this been copied from daf_butler? Don't we have the ability to use the daf_butler callback? If it's simply the integers part shouldn't we implement it on top of the normal splitter with a parameter?
cc/ @n8pease
There was a problem hiding this comment.
It's not an exact copy, daf_butler's one splits things into strings, this one makes integers instead. But I should have probably called split_commas from this method instead of re-implementing its logic.
There was a problem hiding this comment.
Re-reading the question again - I guess we could update split_commas to do type conversion, but my general problem is that click callbacks do not seem to be a good match for that problem. AFAIK callbacks happen after type conversion is already applied to argument, and in our case of comma-separated lists it cannot even work. What I mean is that if you declare an option to have type int then you cannot pass comma-separated string to it because it will try to convert that string to int and will fail. My current solution is not to declare type of an option (essentially making it str) but cheat in the callback and return integers from that. I don't know if this is even allowed, docs don't say if this is acceptable, so there is no guarantee that it will not break.
There was a problem hiding this comment.
Just for record - I reimplemented my _split_commas_int on top of split_commas, for better unification of the latter we may need some thinking, so I want to keep _split_commas_int here for now.
andy-slac
force-pushed
the
tickets/DM-27667
branch
2 times, most recently
from
7aaaa23
to
2971b8e
Compare
andy-slac
force-pushed
the
tickets/DM-27667
branch
2 times, most recently
from
de477bd
to
1c99b9a
Compare
| @@ -42,6 +42,12 @@ def qgraph(pipelineObj, qgraph, skip_existing, save_qgraph, save_single_quanta, | |||
| qgraph : `str` or `None` | |||
| URI location for a serialized quantum graph definition as a pickle | |||
| file. If this option is not None then `pipeline` should be `None`. | |||
| qgraph_id : `str` or `None` | |||
| Quantum graph identifier, graph loaded from a pickle file is required | |||
| to have this identifier. Ignored if graph is not loaded from a file. | |||
There was a problem hiding this comment.
Did you want to change the wording here to match the updated help wording?
New option `--qgraph-node-id` is added which takes a bunch of integers for node numbers to execute from a full graph. Another new option `--qgraph-id` can be used to specify graph ID to verify it after loading from pickle file.
andy-slac
force-pushed
the
tickets/DM-27667
branch
from
1c99b9a
to
b2fc808
Compare
andy-slac
changed the title
New option
--qgraph-node-idis added which takes a bunch of integersfor node numbers to execute from a full graph. Another new option
--qgraph-idcan be used to specify graph ID to verify it after loadingfrom pickle file.