Pull request for Cicero-SCM adapter

[maritsandstad]

Add the Cicero-SCM adapter. With some extra input-files.

• Tests added
• Documentation added
• Example added (in the documentation, to an existing notebook, or in a new notebook) (N/A)
• Description in CHANGELOG.rst added (single line such as: (`#XX <https://github.com/openscm/openscm-runner/pull/XX>`_) Added feature which does something)

[znicholls]

nice stuff @maritsandstad

Is there any way for us to verify that this works or is everything private?

Are you able to rebase this pull request onto master, a few updates have been made

[maritsandstad]

No, I think it should be possible to run it for anyone (I think everything needed is included in the utils_templates subdirectory). It was at least meant to be open, but I might have forgotten some file or other... I will try to look at the rebasing, but probably won't be able to get to it before tomorrow...

[rgieseke]

When you update to the latest changes in OpenSCM-runner you might have to change from using ScmData to ScmRun.

I believe similar to the changes in this commit:

5d44b1f

OpenSCM-runner requires a version of scmdata which no longer has the ScmDataFrame

[rgieseke]

I tried getting this to run, i can run the model binary on my Linux box but i think there are some path issues that need fixing.

The path to '/div/amoc/CSCM/` is hardcoded and should be something relative to the installation path.

In Pyhector we solved this like this for reading an emissions CSV relative to the __iniy__.py file:

https://github.com/openclimatedata/pyhector/blob/master/pyhector/__init__.py#L181

rcp26 = read_hector_input(
    os.path.join(os.path.dirname(__file__), "./emissions/RCP26_emissions.csv")
)

__file__ is the full path of the current file.

Also, i think '/div/amoc/CSCM/Parameters/TilRagnhild2019Juni26/' postTheta.txt might be missing.

Seeing that you managed to get this to finish i might look again into a Pyhector adapter, i tried but gave up after a while 😃

[maritsandstad]

Yes, thank you @rgieseke! I thought I might have missed something. Also thanks for the tip. I will update to fix this... Also, I think the assumption that if I managed anyone can is very sound, go for it ;-)

[maritsandstad]

Ok, one problem, some of the parameter files that I use (like the mentioned 'postTheta.txt') are very large, and exceeds the file size limit of github. I think these will probably have to be shared some other way, to run the bundle. Otherwise, I could in principle "freeze" the parameter choices, but that becomes somewhat inflexible. Suggestions @znicholls or @rgieseke?

[rgieseke]

Depends a bit on the size ... using a subset (which is calibrated to some historical data i assume) would probably be a good solution. This is also similar to what MAGICC does with its 600 parameter set.

Another option could be to deposit the file somewhere (maybe Zenodo) and instruct users within the Python code to set the path to the local copy.

There is also Git LargeFileStorage (Git-LFS) but i don't think that would help with installability.

[znicholls]

Right this is going to be a bit of a pain for you, but you need to re-write how your adapter gets its parameter set. The parameter set shouldn't be inbuilt, rather it should be passed to the adapter via the cfgs argument of run. That means we decouple the config from the run code.

So, please don't put that massive file in the repo. This test of MAGICC is probably the best example. We want to be able to pass in a list of dictionaries with your config, and then let your adapter take care of it from there. How we load the config (e.g. downloading from zenodo or loading a json file or hard-coding into a script) will depend on the use case, it shouldn't be enforced by the openscm_runner package.

[znicholls]

For example, a typical pattern with MAGICC is

# load the config from a json file we've saved previously
configs = json.load(file_with_config.json)

# run magicc
results = run(climate_models_cfgs={ "MAGICC7": configs})

Similarly, we want to be able to do

# load the config from a json file we've saved previously
# this should be relatively small given your probabilistic ensemble
# only has ~600 members?
configs_cicero = json.load(cicero_file_with_config.json)

# run magicc and cicero
results = run(climate_models_cfgs={ "MAGICC7": configs, "Cicero-SCM": configs_cicero})

[znicholls]

One other thing, given your AR6 configs are confidential, definitely do not add them to the repo (that's another reason we decouple config from openscm_runner).

[znicholls]

If you can rebase onto master, the CI should run...

[maritsandstad]

Ok, I think I get how it needs to be done now. I think that should be possible to fix, but will be next week...

[maritsandstad]

Rebase done. I've also figred out how to deal with the configs, but it requires some rewrites, so I'll get onto that now.

[maritsandstad]

I guess I should also reformat using black...?

[rgieseke]

Yes, black helps a lot with being able to concentrate on the code and not on the formatting in projects with multiple contributors. Some editors also have plugins, otherwise you have to use it on the command line.

[maritsandstad]

Black from command line seemed fine. But do I need to double check the version or something, because it keeps failing in the test, even when I've just reformatted it with black from command line...?

[rgieseke]

black seems to be pinned to a specific version in this repo:

black==19.10b0

https://github.com/openscm/openscm-runner/blob/master/setup.py#L42

[rgieseke]

If you use a locally installed, different version, this might make the test fail.
You can install a specific version with pip install black==19.10b0 usually

[maritsandstad]

Ok, now I've removed the offending large files, and rewritten so that the configuration part is taken out, and this is just run with a simple csv-config/input file. All the data now is either in this file (which is basically parameters for the picked ensemble), and a bunch of rcmip inputs to substitute where the scenario input lacks stuff. Finally also our executable binary.

[maritsandstad]

Also, I thought I had the black-issue down with your tips @rgieseke, however, it seems that black and isort disagree on the formating, so whichever one I didn't run last keeps complaining about the same file... (My isort version is 5.5.2 so seems to adhere to the >5 requirement in the setup...)

[rgieseke]

Also, I thought I had the black-issue down with your tips @rgieseke, however, it seems that black and isort disagree on the formating, so whichever one I didn't run last keeps complaining about the same file... (My isort version is 5.5.2 so seems to adhere to the >5 requirement in the setup...)

Yay, these automated tools can be quite helpful, but if the computer knows what to do ... it should just do it 😄 This could actually be part of an automated GitHub Action ...

Anyways, we'll get there!

There sems to be an accidentally committed file slettmeg?

[maritsandstad]

Yes, I thought I took that one out in the last commit. I think it's just got a commit log or something ("slettmeg" literally means "deleteme" in Norwegian).

[rgieseke]

There is also " src/openscm_runner/adapters/ciceroscm_adapter/read_results.py~" with a trailing ~ -- i assume that's a backup file. You can prevent these from being accidentally committed by adding something to a global (or repo-specific) .gitignore file.

For now, you can git rm them.

[maritsandstad]

Ok, that should remove the offending files... Sorry for making so many noob mistakes, and thanks for pointing them out to me!

[rgieseke]

No need to apologise! We've all been there and this project (or family of projects) has its quirks and changes as we go (which is why i gave up with Pyhector at my first try) ...

As for the black/isort conflict:
There are some isort settings in setup.cfg: https://github.com/openscm/openscm-runner/blob/master/setup.cfg#L5

But i'm not sure whether these matter or whether your local isort picks these up when you run it from the project root.

Maybe it helps to completely remove the commented out import (d86625f#diff-114a1890c7e1a741a249681f03f314368880e9627f8daf2a19a29c711ef1e6f3R12)?

[maritsandstad]

I removed the comments, so I think the isort vs black conflict has been resolved... It felt very momentous at this end, but then I still got a fail, so kind of and underwhelming feeling, I guess.... Now it seems I have to find some way of calling the run of my executable without doing it from the shell... Maybe there are some tips in the magicc adapter...

[rgieseke]

Yes, progress!

I don't think it's a problem to skip this security check

Pymagicc solves it here with a #nosec comment: https://github.com/openscm/pymagicc/blob/master/pymagicc/core.py#L361

Adding the same to your subprocess call should hopefully let us get past bandit :-)

[rgieseke]

Or rather os.system: https://github.com/openscm/openscm-runner/pull/24/files#diff-114a1890c7e1a741a249681f03f314368880e9627f8daf2a19a29c711ef1e6f3R93

[rgieseke]

I think it's actually not a security risk as long as openscm-runner is not run as part of a website where malicious users could submit input and craft some special input to hack the server ... but that's probably not happening anytime soon.

[maritsandstad]

@znicholls, rebased ;-)

[maritsandstad]

My local pylint version still doesn't recognise the "consider-using-with" disable keyword in _run_magicc_parallel, but it works here, so I guess it's fine...

[znicholls]

It'll be a version issue (pypi doesn't update automatically), all good. Merge time 🚀

[znicholls]

I'll release a new version tomorrow, well done @maritsandstad, huge effort

[maritsandstad]

Thanks @znicholls, I had a lot of help. (And some fun... occasionally...)

[maritsandstad]

Thanks for all your help, too @rgieseke!

[rgieseke]

Happy to have helped a tiny bit, glad to see it's released! :-)

[znicholls]

0.7.0 is up https://pypi.org/project/openscm-runner/