fix: improved DataDiscoveryCLI interface

[valsdav]

Improving the DataDiscoveryCLI

• do_replicas and load_dataset_definitions now returns uproot compatible metadata
• Added "first" option for rucio replicas
• Returning both overall and updated datasets from preprocess
• Fixes and docs

@iasonkrom

[ikrommyd]

Should we have "first" or "round-robin" be the default though? Opinions?

[valsdav]

round-robin by default distributes the load more between sites.

[ikrommyd]

Does it "reduce" the quality of the sites though? Are "bigger" sites generally better?

[lgray]

Round-robin is fine and probably scales better. If it turns out that it causes problems we can change it later, but I think that gets determined by experimentation rather than how we think it is supposed to work.

[ikrommyd]

One final thing to ask is a method that returns the dictionary that do_save would write to a json file. Maybe .as_dict like @lgray mentioned in Slack. After that, it looks good to me!

[lgray]

Either as_dict or to_dict would be fine, but yes, that's needed @valsdav .

[ikrommyd]

Oh I got one more thought. What if we also keep out_available and out_updated dictionaries as class attributes if preprocess was run? Just to have direct access to them from the class and not only from the json files.

[ikrommyd]

self.preprocessed_available not preprocess_available

[ikrommyd]

same here

[ikrommyd]

same here

[lgray]

@valsdav @iasonkrom ping me when you've converged, and I'll merge it.

[ikrommyd]

@lgray do_preprocess() takes in dask_cluster=None and spawns a with Client(dask_cluster) as _ to do the preprocessing. But what if the user already has a Client running? No reason to spawn another one right? I'm good with all the rest

[ikrommyd]

I don't know if passing in dask_client and not dask_cluster is the correct approach here

[valsdav]

if you provide a dask scheduler url the Client is going to connect to that. if None it will spawn a local multiprocessing cluster, isn't it?

[ikrommyd]

I'm not sure about that. Lindsey will have to confirm that it's fine. I'm good with all the rest. @lgray you're free to review

[lgray]

That's all the functionality we should expect in a CLI system, see https://distributed.dask.org/en/stable/client.html.

So I think we can provide the scheduler endpoint as a string and call it a day here.

If you need to do more complicated preprocessing, you can get the raw list and do what you want, or call the pre-processing command in a script in the appropriate context manager with your dask cluster setup.

What's there is fit to task.

[lgray]

@valsdav change dask_cluster to scheduler_url and I think the usage becomes more clear.

[ikrommyd]

so if I've spawned an lpc condor client with:

from distributed import Client
from lpcjobqueue import LPCCondorCluster


cluster = LPCCondorCluster()
cluster.adapt(minimum=0, maximum=100)
client = Client(cluster)

what should I do to make do_preprocess use that Client?

[lgray]

as_dict -> call coffea.dataset_tools.preprocess

[ikrommyd]

as_dict -> call coffea.dataset_tools.preprocess

ah okay, so we just don't use the CLI class's method in that case. Thanks

I'm good with merging as soon as you review! Thanks a lot @valsdav!