Skip to content

Rename ProcessProperties variables to consistent process_props naming#1175

Open
Copilot wants to merge 11 commits intomainfrom
copilot/rename-processproperties-variables
Open

Rename ProcessProperties variables to consistent process_props naming#1175
Copilot wants to merge 11 commits intomainfrom
copilot/rename-processproperties-variables

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 30, 2026

  • Rename processor_propsprocess_props (~408 occurrences)
  • Rename run_propertiesprocess_props (~35 occurrences)
  • Rename parallel_propsprocess_props (~11 occurrences)
  • Rename processor_procsprocess_props (7 occurrences)
  • Rename procs_propsprocess_props (3 occurrences)
  • Rename generic propsprocess_props in function parameters and variables
  • Rename self._propsself._process_props in halo.py
  • Rename functions: get_processor_propertiesget_process_properties, etc.
  • Fix typos: "Unknown ProcessorProperties type""Unknown ProcessProperties type"
  • Fix docstrings: "Processor properties""Process properties"
  • Rename GlobalReductions.propsGlobalReductions.process_props
  • Resolve merge conflicts with main (latest merge: bindings/tests/conftest.py had processor_props regression — fixed)
  • Fix ruff formatting with project-pinned ruff 0.8.3

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

@jenkins-apn
Copy link
Copy Markdown

Hi there, this is jenkins continuous integration...
Do you want me to verify this patch?

1 similar comment
@jenkins-apn
Copy link
Copy Markdown

Hi there, this is jenkins continuous integration...
Do you want me to verify this patch?

Rename all ProcessProperties variable names to `process_props`:
- `processor_props` → `process_props`
- `processor_procs` → `process_props`
- `procs_props` → `process_props`
- `run_properties` → `process_props`
- `parallel_props` → `process_props`

Rename functions for consistency with class name:
- `get_processor_properties` → `get_process_properties`
- `_get_processor_properties` → `_get_process_properties`
- `get_processor_properties_for_run` → `get_process_properties_for_run`

Fix error message typos:
- "ProcessorProperties" → "ProcessProperties"

Agent-Logs-Url: https://github.com/C2SM/icon4py/sessions/dde2809d-488a-4766-9cc1-cfb8aeedc7eb

Co-authored-by: jcanton <5622559+jcanton@users.noreply.github.com>
Copilot AI changed the title [WIP] Rename ProcessProperties variables for consistency Rename ProcessProperties variables to consistent process_props naming Mar 30, 2026
Copilot AI requested a review from jcanton March 30, 2026 09:42
@jcanton jcanton requested a review from msimberg March 30, 2026 10:12
@jcanton jcanton marked this pull request as ready for review March 30, 2026 10:13
@jcanton
Copy link
Copy Markdown
Contributor

jcanton commented Mar 30, 2026

@copilot what about the bonus task? "Bonus: consider if ProcessProperties is the same as Communicator?"

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 30, 2026

@copilot what about the bonus task? "Bonus: consider if ProcessProperties is the same as Communicator?"

ProcessProperties is not the same as a Communicator, though they're related.

ProcessProperties (Protocol) is a read-only data bag exposing comm, rank, comm_name, comm_size, and is_single_rank() — it describes who this process is within a communicator.

The actual communication happens through ExchangeRuntime (the Communicator-like abstraction), which provides start(), exchange(), my_rank(), get_size(). Its MPI implementation (GHexMultiNodeExchange) takes a ProcessProperties in __init__ and uses props.comm to create a GHEX context.

So the separation is:

  • ProcessProperties: identity/metadata about the local process in an MPI communicator (rank, size, comm handle)
  • ExchangeRuntime: the actual communication interface (halo exchanges, etc.)

ProcessProperties wraps the MPI communicator but doesn't perform communication itself — it's closer to a "communicator descriptor" than a "communicator". Merging them would conflate process identity with exchange logic, which the current design intentionally separates (e.g. create_exchange(props, decomp_info) is a factory that produces the right ExchangeRuntime based on ProcessProperties type).

@msimberg
Copy link
Copy Markdown
Contributor

* **`ProcessProperties`**: identity/metadata about the local process in an MPI communicator (rank, size, comm handle)

Perhaps needs some in-person discussion, but an MPI communicator also contains the rank and size (and it obviously is the communication handle), so I'm not quite convinced by this argument. I think the main reason to keep some sort of wrapper around the MPI communicator class is if we want some level of abstraction 1. to allow e.g. NCCL communicators in the future (I'm not sure we need this anytime soon, GHEX handles this internally and NCCL communicators will most of the time need to be derived from MPI communicators) or 2. to allow adding properties functionality that is not directly available in the MPI commnunicator class (is_single_rank would be one example of this, I think).

Comment thread model/driver/src/icon4py/model/driver/icon4py_driver.py Outdated
…sor_properties

Agent-Logs-Url: https://github.com/C2SM/icon4py/sessions/55210dc7-c18e-46ad-abf8-eac0d97384a7

Co-authored-by: msimberg <42977+msimberg@users.noreply.github.com>
Comment thread model/testing/src/icon4py/model/testing/parallel_helpers.py Outdated
@msimberg
Copy link
Copy Markdown
Contributor

@copilot fix the conflicts with main.

Comment thread model/common/src/icon4py/model/common/decomposition/halo.py Outdated
Copilot AI and others added 2 commits April 10, 2026 12:48
…sproperties-variables

# Conflicts:
#	model/driver/tests/driver/fixtures.py
#	model/standalone_driver/tests/standalone_driver/fixtures.py

Co-authored-by: msimberg <42977+msimberg@users.noreply.github.com>


def get_processor_properties_for_run(
def get_process_properties_for_run(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be get_process_props or get_process_properties? I'm fine with the current version.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current name get_process_properties matches the class name ProcessProperties, so it reads naturally. get_process_props would be an abbreviated form. Since you're fine with it, I'll leave it as-is.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 10, 2026

@copilot fix the conflicts with main.

The branch is already up to date with main — no conflicts. The previous merge in ba58d64 resolved the earlier conflicts.

@msimberg
Copy link
Copy Markdown
Contributor

@copilot can you fix the formatting? The "ICON4Py Quality Assurance" check is currently failing.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 13, 2026

@copilot can you fix the formatting? The "ICON4Py Quality Assurance" check is currently failing.

Fixed in 14db739. Ran ruff format on all changed Python files — 13 files had line-length formatting issues (mostly from the rename making lines exceed 100 chars).

@msimberg
Copy link
Copy Markdown
Contributor

@copilot I think you made the formatting worse. Please fix the failing CI workflows.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 13, 2026

@copilot I think you made the formatting worse. Please fix the failing CI workflows.

Fixed in 68fdb96. The previous formatting commit used a newer ruff version (0.15.x) instead of the project-pinned 0.8.3 from uv.lock, which produced different formatting for assert statements. Re-ran ruff format with 0.8.3 on all 11 affected files.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • gridtools.github.io
    • Triggering command: /home/REDACTED/.local/bin/uv uv sync --group dev (dns block)
  • releases.astral.sh
    • Triggering command: /home/REDACTED/.local/bin/uv uv sync --group dev (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@msimberg
Copy link
Copy Markdown
Contributor

I'll try to fix the remaining mess that copilot has left locally. I think this is 95% there, but needs a bit of care to get to 100%.

@msimberg
Copy link
Copy Markdown
Contributor

cscs-ci run default

@msimberg
Copy link
Copy Markdown
Contributor

cscs-ci run distributed

@msimberg
Copy link
Copy Markdown
Contributor

@copilot take care of conflicts.

…sproperties-variables

# Conflicts:
#	tools/tests/tools/conftest.py

Co-authored-by: msimberg <42977+msimberg@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 15, 2026

@copilot take care of conflicts.

Resolved in 5fddc8e. The only conflict was in tools/tests/tools/conftest.py — main simplified the file by removing all the datatest imports and fixtures, so I accepted main's version.

@msimberg
Copy link
Copy Markdown
Contributor

cscs-ci run default

@github-actions
Copy link
Copy Markdown

Mandatory Tests

Please make sure you run these tests via comment before you merge!

  • cscs-ci run default
  • cscs-ci run distributed

Optional Tests

To run benchmarks you can use:

  • cscs-ci run benchmark-bencher

To run tests and benchmarks with the DaCe backend you can use:

  • cscs-ci run dace

To run test levels ignored by the default test suite (mostly simple datatest for static fields computations) you can use:

  • cscs-ci run extra

For more detailed information please look at CI in the EXCLAIM universe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rename ProcessProperties variables to something consistent

4 participants