feat: add support for REANA - #231
Conversation
| - python3 ttbar_analysis_pipeline.py | ||
| outputs: | ||
| files: | ||
| - histograms.root |
There was a problem hiding this comment.
I think workspace.json would be another good addition here as a product of the workflow (it contains the statistical model).
| @@ -0,0 +1,38 @@ | |||
| inputs: | |||
| files: | |||
There was a problem hiding this comment.
A few of the files here aren't technically needed I think, like nanoad_branch_ratios.json, GetIOBranches.py, jetassignment_training* and make_corrections_json.
There was a problem hiding this comment.
Yes, indeed most of the inputs were unnecessary. Updated the inputs list.
alexander-held
left a comment
There was a problem hiding this comment.
Thanks for adding this, great to see this functionality on REANA! I added two suggestions, let me know what you think but I'm happy to merge either way.
| resources: | ||
| kerberos: true | ||
| dask: | ||
| image: registry.cern.ch/docker.io/alputer/agc-dask:1.0.0 |
There was a problem hiding this comment.
If the analysis has N_FILES_MAX_PER_SAMPLE = 5 by default, we don't have to have so much Dask workers.
Let's tune the parameters such as number_of_workers down so that the default reana.yaml would be executable even on a laptop.
(And we could recommend "big" settings for the full data processing somewhere in the docs.)
There was a problem hiding this comment.
Yes, I forgot that N_FILES_MAX_PER_SAMPLE = 5 by default. I will scale down the cluster and update reana.yaml accordingly.
There was a problem hiding this comment.
I scaled down the requested cluster size, but I guess with default settings it is not runnable locally even with N_FILES_MAX_PER_SAMPLE = 5. I tried the following combinations and the cluster was under a lot of pressure and execution was extremely slow. So I think we should use more powerful clusters in any case ...
Option 1 (Was very slow, at least half an hour execution time even with INPUT_FROM_EOS=True)
number_of_workers = 2
single_worker_memory = 2Gi
Option 2 (Workers under a lot of pressure, losing progress and likely to fail)
number_of_workers = 5
single_worker_memory = 1Gi
There was a problem hiding this comment.
FWIW here is one data point. Testing on a laptop with 12 CPU cores and 32 GB RAM, using INPUT_FROM_EOS being True. (So the run includes inference etc.) Here are results for 5 Dask workers of 2 GiB memory and 1 thread (execution time 15 min 40 sec) and then for 10 Dask workers (10 min 28 sec). (Kerberos was off in both cases; enabling it could give further speed-up.)
P.S. A further test with 15 workers showed similar time as with 10 workers, which makes perfect sense, because the laptop has only 12 cores.
| workflow: | ||
| type: serial | ||
| resources: | ||
| kerberos: true |
There was a problem hiding this comment.
kerberos: True could be bothersome for people that would be running this example outside of CERN. So I guess we should leave it out, and just document (e.g. in the full run docs) that setting Kerberos helps with faster data access.
| cluster.scale(10) | ||
| client = cluster.get_client() | ||
|
|
||
| elif af == "reana": |
There was a problem hiding this comment.
Is the new analysis facility necessary? Perhaps not if we do something like:
DASK_SCHEDULER_URI = os.getenv("DASK_SCHEDULER_URI", "tcp://127.0.0.1:8080")
client = dask.distributed.Client(DASK_SCHEDULER_URI)I.e. read the environment variable to know where to connect, and if there is none set, then use the Dask defaults?
Such a technique could perhaps work for both REANA use case and the local use case. (Untested.)
One advantage this would bring is to "teach" researchers how to write Dask code runnable both locally and on REANA from the start.
There was a problem hiding this comment.
I think it might be difficult to fully generalize the logic: on some facilities we expect to connect to an existing cluster, which may or may not have an addressed stored in an environment variable, but then if we do not find that the right action to take still depends on the facility as the spawning varies as well. I'm open to suggestions though.
We could add the logic at the beginning of the function to connect to an existing cluster if the environment variable points to one and ignore the value of af in this case but I'm not sure if that would be easier to follow.
There was a problem hiding this comment.
I think without introducing a new analysis facility, code will be less intuitive as all different ways to run and connect to Dask clusters are handled in seperate if clauses, and handling REANA and local use cases together will be an exception to that.
There was a problem hiding this comment.
Yeah, I thought above only about "local" and "reana", that these two could perhaps be one. (Because the only thing that is different is the URI of the Dask scheduler.) But for the other analysis facilities, and their special spawning, would have to stay apart...
There was a problem hiding this comment.
@alexander-held BTW it would be very nice to make the analysis fully configurable so that people wouldn't have to edit utils/config.py for things such as selecting analysis facility or whether inference should be done or not.
This could be done by two ways, either respecting command-line arguments to the notebook in the papermill style, or reading values from environment variables if they are set. For example:
# a) using command-line options
$ python3 ttbar_analysis_pipeline.py --use-inference False
# b) using environment variables
$ USE_INFERENCE=False python3 ttbar_analysis_pipeline.pyWould you be open for such changes to the repository? We could discuss IRL on Friday.
There was a problem hiding this comment.
For the af handling: I would propose to stick with the implementation we have instead of partially merging logic as I'm afraid that may only make it more complicated to follow.
For the topic of configuring things: yes, that sounds like a good idea to me. There are a few things in the main file
utils/config.py. We probably do not need to support configuring all of them externally but the performance-related ones would be useful. A CLI feels more pythonic to me than picking up environment variables and would still allow env-based configuration by just feeding those variables in through the CLI. We might need to think a bit about how to integrate this in a way that is not too invasive to keep the notebook / .py synchronization working but I'm sure that we can find a solution there.
Add REANA as an alternative way to run Dask workflows. REANA brings up a dedicated Dask cluster for workflow and shuts it down after the workflow is finished.
3967db3 to
1f51b54
Compare

Add REANA as an alternative way to run the workflow with Dask. REANA brings up a dedicated Dask cluster for workflow and shuts it down after the workflow is finished.
docker.io/coffeateam/coffea-base-almalinux9:head-py3.10. Will be updated with an official image.