DM-42631: Add CC-IN2P3 (Slurm) as a target site #28
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
==========================================
- Coverage 47.46% 47.20% -0.27%
==========================================
Files 11 11
Lines 375 375
Branches 61 61
==========================================
- Hits 178 177 -1
+ Misses 191 189 -2
- Partials 6 9 +3 ☔ View full report in Codecov by Sentry. |
python/lsst/ctrl/bps/parsl/site.py
Outdated
| """Get Parsl configuration for this site. | ||
|
|
||
| This method allows concrete subclasses to override this method to | ||
| provide a a Parsl configuration specific for the site. If not |
| @@ -52,6 +52,10 @@ def get_parsl_config(config: BpsConfig) -> parsl.config.Config: | |||
| For details on the site configuration, see `SiteConfig`. For details on the | |||
| monitor configuration, see ``get_parsl_monitor``. | |||
|
|
|||
| Subclasses of `SiteConfig` can override their method ``get_parsl_config`` | |||
| to configure Parsl for the specific context of the site. A default | |||
| Parsl configuration is returned if the subclass did not provide a config. | |||
There was a problem hiding this comment.
This is something that should get added to the docs.
| @@ -65,11 +69,14 @@ def get_parsl_config(config: BpsConfig) -> parsl.config.Config: | |||
| Parsl configuration. | |||
| """ | |||
| site = SiteConfig.from_config(config) | |||
| executors = site.get_executors() | |||
| retries = get_bps_config_value(site.site, "retries", int, 1) | |||
| monitor = site.get_monitor() | |||
There was a problem hiding this comment.
There's no need to change this. Just stick the new lines in above.
There was a problem hiding this comment.
Reverted. I kept the previous style.
| retries = get_bps_config_value(site.site, "retries", int, 1) | ||
| monitor = site.get_monitor() | ||
| if parsl_config := site.get_parsl_config(): | ||
| return parsl_config |
There was a problem hiding this comment.
The new get_parsl_config in Ccin2p3 replicates the below. Why not have get_parsl_config return values which will be used to supplement/override the ones constructed here?
There was a problem hiding this comment.
Added a method get_parsl_config() to base class SiteConfig, overwritable by subclasses. This seems to me cleaner now.
…figuration and support Parsl configuration attributes to be specified in submission file.
pre-commit autoupdate
|
@PaulPrice Many thanks for your feedback. I think I implemented your recommendations. You may want to check the final result before I merge. |
| # Path to Parsl run directory. The default set by Parsl is | ||
| # 'runinfo' which is not explicit enough for end users given that | ||
| # we are using BPS + Parsl + Slurm to execute a workflow. | ||
| run_dir = get_bps_config_value(self.site, "run_dir", str, "parsl_runinfo") |
There was a problem hiding this comment.
Oh, this is a nice feature! It might be good to hoist this into the parent class.
There was a problem hiding this comment.
I can add that feature to the base class but I suggest we keep runinfo as the default value (i.e. the default value set by Parsl), to keep backwards compatibility for other sites (e.g. Princeton).
|
Thanks, it looks good. It would be helpful if you could clean up the commit history some, as it looks like there are more commits than changes in the end. |
|
I tried to clean the history but failed. Apologies for that. It became a mess: my knowledge of git was not enough to sort things out in a reasonable amount of time, so I preferred to get the code merged as is. Lesson learned for future contributions, hopefully. |
Checklist
doc/changes