update for pycopanlpjml by jnnsbrr · Pull Request #17 · PIK-LPJmL/pycoupler

jnnsbrr · 2026-02-05T16:11:07Z

This MR introduces new features for the compatibility with the latest pycopanlpjml version:

default model name set to copan, instead of copan:CORE - if provided via CoupledConfig it can be set
functionality to kill remaining processes on coupled port to avoid blocked port in the next simulation
debug historic years reading for coupling
adjust slurm.jcf file creation for older LPJmL versions to avoid slurm ending the job before coupled program has finished
deprecation warning for old objects names, attributes

…, fix for start_lpjml to not close coupled program to early, deprecation warning function

codecov · 2026-02-06T08:59:39Z

Codecov Report

❌ Patch coverage is 82.87037% with 74 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.43%. Comparing base (9490c5e) to head (bbf7b67).

Files with missing lines	Patch %	Lines
pycoupler/coupler.py	70.65%	27 Missing ⚠️
pycoupler/data.py	87.36%	24 Missing ⚠️
pycoupler/run.py	85.71%	16 Missing ⚠️
pycoupler/config.py	65.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #17      +/-   ##
==========================================
+ Coverage   78.47%   81.43%   +2.96%     
==========================================
  Files           7        7              
  Lines        1640     1988     +348     
==========================================
+ Hits         1287     1619     +332     
- Misses        353      369      +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR updates pycoupler for compatibility with newer pycopanlpjml/LPJmL coupling expectations, including a new default coupled model name, improved Slurm coupled-job handling, additional port/process cleanup, and expanded test coverage for utilities, run submission, and NetCDF export helpers.

Changes:

Default coupled model name updated to copan, with support for overriding via a coupled config.
Slurm coupled-job submission now patches slurm.jcf to ensure the coupler process is waited on (and submits via sbatch when needed).
Adds NetCDF writing + grid transform utilities for LPJmLData/LPJmLDataSet, plus substantial new tests.

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
`pycoupler/config.py`	Adjusts coupled config defaults/behavior (model default + coupled config support).
`pycoupler/coupler.py`	Adds port cleanup utilities; fixes historic years handling; improves input sending conversion logic.
`pycoupler/run.py`	Introduces `start_lpjml` and deprecated alias; patches Slurm submission flow for coupled runs.
`pycoupler/utils.py`	Adds standardized deprecation warning helper.
`pycoupler/data.py`	Implements `transform()` and NetCDF export helpers for LPJmL data structures.
`tests/test_config.py`	Updates expectations for new default coupled model.
`tests/test_couple.py`	Adjusts time assignment and historic years expectation in coupling test.
`tests/test_run.py`	Extends Slurm submission tests to cover `slurm.jcf` patching and `sbatch` flow.
`tests/test_run_additional.py`	Adds additional run-related tests incl. deprecated alias warning behavior.
`tests/test_utils.py`	Extends `detect_io_type` test coverage for invalid UTF-8 bytes.
`tests/test_utils_additional.py`	Adds coverage for `create_subdirs` and `read_json`.
`tests/test_data.py`	Adds comprehensive coverage for grid transforms and NetCDF writing.
`tests/test_coupler_utils.py`	Adds coverage for new port cleanup utilities.
`tests/data/invalid_utf8.bin`	Adds a sample binary file (currently appears redundant with the updated test).
`pycoupler/release.py`	Clarifies release script instructions.
`pycoupler/__init__.py`	Minor formatting cleanup.
`CITATION.cff`	Bumps version to `1.7.0`.

Comments suppressed due to low confidence (1)

pycoupler/run.py:265

slurm_jcf_dir is documented/used as the directory where slurm.jcf should be written, but the lpjsubmit subprocess is still executed without cwd=.... If lpjsubmit writes slurm.jcf to its working directory (typical), _patch_slurm_and_submit() will look in slurm_jcf_dir/slurm.jcf and fail even though the file exists elsewhere. Consider ensuring the directory exists and running lpjsubmit with cwd=slurm_jcf_dir (or otherwise directing lpjsubmit output) so slurm_jcf_path matches where the file is actually created.

    # run in coupled mode and pass coupling program/model
    needs_coupler_wait = bool(couple_to)
    if slurm_jcf_dir is None:
        slurm_jcf_dir = os.getcwd()
    slurm_jcf_path = Path(slurm_jcf_dir) / "slurm.jcf"

    couple_file = None

    if couple_to:
        python_path = "python3"
        if venv_path:
            python_path = os.path.join(venv_path, "bin/python")
            if not os.path.isfile(python_path):
                raise FileNotFoundError(
                    f"venv path contains no python binary at '{python_path}'."
                )

        bash_script = f"""#!/bin/bash

# Define the path to the config file
config_file="{config_file}"

# Call the Python script with the config file as an argument
{python_path} {couple_to} $config_file
"""

        couple_file = f"{output_path}/copan_lpjml.sh"

        with open(couple_file, "w") as file:
            file.write(bash_script)

        # Change the permissions of the file to make it executable
        run(["chmod", "+x", couple_file])

        cmd.extend(["-couple", couple_file])

    if needs_coupler_wait:
        cmd.append("-norun")

    cmd.extend([str(ntasks), config_file])

    # Intialize submit_status in higher scope
    submit_status = None
    # set LPJROOT to model_path to be able to call lpjsubmit
    try:
        os.environ["LPJROOT"] = config.model_path
        # call lpjsubmit via subprocess and return status if successfull
        submit_status = run(cmd, capture_output=True)
    except Exception as e:

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

sieben-gea

This pull requests tries to do too many things at once. Please, in the future, split many unrelated changes like these into multiple PRs and, at the very least, into meaningful commits with proper commit messages.

I found that the code lacks coherence and has many subtle problems. In particular the changes to data.py seem half-done to me. It might be that those changes work in your particular use case, but they are not general enough to be used in production.

Most of the tests you submitted are also less meaningful then they might seems. Many of the only test if a particular function is called, which ignores their side effects and can not be considered a full test.

Please fully read all the changes you made and explain to me what you were trying to do (and also how these changes are related to the changes in pycopanlpjml, because I don’t yet see how).

sieben-gea · 2026-04-09T10:28:46Z

        for inp in inputs:
            sock_input = getattr(self.input, inp)
+            if not hasattr(sock_input, "__dict__"):
+                continue  # skip scalars (e.g. delta_year in LPJmL v6)


I don’t understand this? What changed in LPJmL v6?

sieben-gea · 2026-04-09T10:37:52Z

@@ -558,6 +571,8 @@ def _set_input_sockets(self, inputs=[]):
        """Set sockets for inputs and outputs (via corresponding ids)"""
        for inp in inputs:
            sock_input = getattr(self.input, inp)


Where is self.input set? We depend on it being in the config, but there is no method that sets it.

sieben-gea · 2026-04-09T12:09:34Z

-    def regrid(self, sim_path, model_path=None, country_code="BEL", overwrite=False):
+    def regrid(
+        self, sim_path, model_path=None, country_code="BEL", overwrite=False
+    ):  # noqa: E501


We should disable this error globally. It makes no sense, especially since we already enforce formatting through black (and I also am not a fan of enforcing such a short line length, because I like to fit more code on my screen and this is trading off vertical space with horizontal space). PEP 8 allows up to 99 characters in a line if this is consistent within a team.

sieben-gea · 2026-04-09T13:53:15Z

+    try:
+        # Find processes using the port
+        result = subprocess.run(
+            ["lsof", "-ti", f":{port}"], capture_output=True, text=True, timeout=5


This is way to broad. Killing processes of other users is not acceptable, especially on the cluster. Try something more specific, like:

lsof -u $USER -a -i :<PORT NUMBER> -sTCP:LISTEN -t

Another thought: Why is a port even needed? If we communicate via sockets anyway, we should use socket files on linux (as discussed here: https://serverfault.com/questions/195328/unix-socket-vs-tcp-ip-hostport).

sieben-gea · 2026-04-13T07:12:42Z

+        if result.returncode == 0 and result.stdout.strip():
+            pids = result.stdout.strip().split("\n")
+            killed_count = 0
+            for pid in pids:
+                if pid.strip():
+                    try:
+                        kill_result = subprocess.run(
+                            ["kill", "-9", pid.strip()],
+                            timeout=5,
+                            capture_output=True,
+                        )
+                        if kill_result.returncode == 0:
+                            killed_count += 1
+                    except subprocess.TimeoutExpired:


All of this could have been a pipe: lsof … -t | xargs kill -9

sieben-gea · 2026-06-26T14:46:29Z

+                    "Cannot write LPJmLData with a 'cell' dimension that lacks "  # noqa: E501
+                    "'lon' and 'lat' coordinates."
+                )
+            lpjml = lpjml.transform("lon_lat")


This is transforming the instance itself!

sieben-gea · 2026-06-26T14:53:17Z

+        _suppress_coordinate_fill(dataset)
+        if global_attrs:
+            dataset.attrs.update(dict(global_attrs))
+        dataset.attrs.setdefault("Conventions", "CF-1.8")


Why is this not part of _ensure_cf_metadata() and have you checked this version is actually the one adhered to?

sieben-gea · 2026-06-26T15:12:47Z

+        kwargs = dict(kwargs)
+        per_variable = kwargs.pop("split_vars", per_variable)
+        kwargs.pop("file_prefix", None)
+        kwargs.pop("suffix", None)


Why do you remove these?

sieben-gea · 2026-06-26T15:16:19Z

    return LPJmLMetaData(read_json(file_name))


+def _netcdf_encoding(


If you already created a function for this, please use it in LpjmlData as well.

sieben-gea · 2026-06-26T15:17:46Z

+
+    aligned = xr.Dataset(data_vars)
+    aligned.attrs.update(ds.attrs)
+    aligned.attrs.setdefault("Conventions", "CF-1.8")


Why would you do this here?

jnnsbrr added 8 commits January 8, 2026 17:55

model_name support, port clean up, historic_years fix, netcdf support…

21c2cc2

…, fix for start_lpjml to not close coupled program to early, deprecation warning function

Merge branch 'main' into docs

d708a0f

Version 1.7.0

2bcd3eb

fix linting

fedea86

fix tests

3e9ecbe

fix tests

4e71ae7

fix tests

4aa5e9a

fix tests

a8c3516

jnnsbrr added 3 commits February 6, 2026 10:19

fix black

8755e24

fix black

86cf76e

add more tests

8038fe8

jnnsbrr requested review from Copilot and zner0L and removed request for Copilot February 6, 2026 12:41

Copilot started reviewing on behalf of jnnsbrr February 6, 2026 12:42 View session

jnnsbrr changed the title ~~pycopanlpjml update~~ update for pycopanlpjml Feb 6, 2026

Copilot AI reviewed Feb 6, 2026

View reviewed changes

jnnsbrr mentioned this pull request Feb 10, 2026

Introducing countries as regions, parallelization and advancing the "earth API" pik-copan/pycopanlpjml#5

Open

jnnsbrr added 7 commits February 12, 2026 16:56

fix pycoupler for lpjml v6

e8c74d1

fix linting

b6b9dcf

fix for lpjml v6

f4e4185

linter fixes

a143b0a

fix tests

0f1e3d9

fix copilot remarks, add tests and linter fixes

bbf7b67

kill process of port to free port again

49efe99

sieben-gea requested changes Jun 26, 2026

View reviewed changes

		return LPJmLMetaData(read_json(file_name))


		def _netcdf_encoding(

Uh oh!

Conversation

jnnsbrr commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sieben-gea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jnnsbrr commented Feb 5, 2026 •

edited

Loading

codecov Bot commented Feb 6, 2026 •

edited

Loading