Skip to content

fix(user_tools): replace deprecated prop containers - #2105

Open
WilliamK112 wants to merge 4 commits into
NVIDIA:devfrom
WilliamK112:codex/fix-deprecated-prop-container
Open

fix(user_tools): replace deprecated prop containers#2105
WilliamK112 wants to merge 4 commits into
NVIDIA:devfrom
WilliamK112:codex/fix-deprecated-prop-container

Conversation

@WilliamK112

@WilliamK112 WilliamK112 commented Jun 23, 2026

Copy link
Copy Markdown

Description

Replace the remaining in-memory JSONPropertiesContainer usages in Dataproc and cluster inference with AbstractPropContainer, removing the runtime DeprecationWarning while preserving existing property access and JSON-string handling.

The migration now covers:

  • inferred cluster properties
  • Dataproc master and worker node properties
  • Dataproc pricing configuration
  • Dataproc instance descriptions

Regression coverage exercises all four paths, verifies the new container type and existing outputs, and confirms that the deprecated-container warning is not emitted.

Fixes #1898.

Tests

  • PYTHONPATH=src .venv/bin/python -m pytest -q tests/spark_rapids_tools_ut/test_deprecated_prop_container.py (4 passed)
  • .venv/bin/python -m pylint -d fixme --load-plugins pylint_pydantic --rcfile=../.pylintrc src/spark_rapids_pytools/cloud_api/dataproc.py src/spark_rapids_pytools/common/cluster_inference.py tests/spark_rapids_tools_ut/test_deprecated_prop_container.py (10.00/10)
  • .venv/bin/python -m flake8 src/spark_rapids_pytools/cloud_api/dataproc.py src/spark_rapids_pytools/common/cluster_inference.py tests/spark_rapids_tools_ut/test_deprecated_prop_container.py
  • .venv/bin/python -m py_compile src/spark_rapids_pytools/cloud_api/dataproc.py src/spark_rapids_pytools/common/cluster_inference.py tests/spark_rapids_tools_ut/test_deprecated_prop_container.py
  • git diff --check upstream/dev...HEAD

Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
@github-actions github-actions Bot added the user_tools Scope the wrapper module running CSP, QualX, and reports (python) label Jun 23, 2026
@greptile-apps

greptile-apps Bot commented Jun 23, 2026

Copy link
Copy Markdown

Greptile Summary

This PR replaces two in-memory JSONPropertiesContainer usages — Dataproc worker/master node initialization in DataprocCluster._init_nodes and cluster inference in ClusterInference.infer_cluster — with the preferred AbstractPropContainer, eliminating the DeprecationWarning on those paths. A new test file validates that both paths produce AbstractPropContainer instances and emit no deprecation warnings.

  • dataproc.py: two JSONPropertiesContainer(prop_arg=…, file_load=False) calls replaced with AbstractPropContainer(props=…); the API surface (get_value, get_value_silent) is identical so no downstream behaviour changes.
  • cluster_inference.py: single replacement in infer_cluster; equally safe.
  • Three additional JSONPropertiesContainer usages remain in dataproc.py (pricing config, instance descriptions, and a type annotation on _set_zone_from_props), which will continue to emit the deprecation warning on those code paths.

Confidence Score: 4/5

Safe to merge — the two targeted call-sites are correctly migrated and the API contract is preserved; the remaining deprecated usages in the same file are pre-existing and unrelated to this change's correctness.

The substitution from JSONPropertiesContainer to AbstractPropContainer is straightforward: both expose identical get_value / get_value_silent methods, and the new class is a Pydantic BaseModel that accepts Any for its props field, so no downstream call-sites break. The new regression tests verify correct types and absence of warnings on both fixed paths. The only gap is that three other JSONPropertiesContainer usages remain in dataproc.py, meaning the deprecation warning still fires on the pricing and instance-description paths — a known scope limitation of this PR.

dataproc.py still has three un-migrated JSONPropertiesContainer usages that will continue emitting deprecation warnings on the pricing config and instance-description code paths.

Important Files Changed

Filename Overview
user_tools/src/spark_rapids_pytools/cloud_api/dataproc.py Replaces two in-memory JSONPropertiesContainer usages (worker and master node props in _init_nodes) with AbstractPropContainer, but three additional deprecated usages remain: pricing_config construction (~line 118), raw_instances_descriptions (~line 323), and the _set_zone_from_props type annotation (~line 531).
user_tools/src/spark_rapids_pytools/common/cluster_inference.py Cleanly replaces the single JSONPropertiesContainer usage in infer_cluster with AbstractPropContainer; API surface (get_value, get_value_silent) is identical so the substitution is safe.
user_tools/tests/spark_rapids_tools_ut/test_deprecated_prop_container.py New regression test file covering both reported deprecation paths; uses warnings.catch_warnings to assert no deprecated-container warning is raised, and verifies AbstractPropContainer instance types on the returned objects.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant DPC as DataprocCluster._init_nodes
    participant CI as ClusterInference.infer_cluster
    participant APC as AbstractPropContainer
    participant DN as DataprocNode

    DPC->>APC: "AbstractPropContainer(props=raw_worker_prop)"
    APC-->>DPC: worker_props container
    DPC->>DN: "set_fields_from_dict({props: worker_props, ...})"
    DN->>APC: get_value_silent('machineTypeUri')
    APC-->>DN: instance type info

    DPC->>APC: "AbstractPropContainer(props=raw_master_props)"
    APC-->>DPC: master_props container
    DPC->>DN: "set_fields_from_dict({props: master_props, ...})"

    CI->>APC: "AbstractPropContainer(props=cluster_conf)"
    APC-->>CI: cluster_props_new
    CI->>CI: "platform.load_cluster_by_prop(cluster_props_new, is_inferred=True)"
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant DPC as DataprocCluster._init_nodes
    participant CI as ClusterInference.infer_cluster
    participant APC as AbstractPropContainer
    participant DN as DataprocNode

    DPC->>APC: "AbstractPropContainer(props=raw_worker_prop)"
    APC-->>DPC: worker_props container
    DPC->>DN: "set_fields_from_dict({props: worker_props, ...})"
    DN->>APC: get_value_silent('machineTypeUri')
    APC-->>DN: instance type info

    DPC->>APC: "AbstractPropContainer(props=raw_master_props)"
    APC-->>DPC: master_props container
    DPC->>DN: "set_fields_from_dict({props: master_props, ...})"

    CI->>APC: "AbstractPropContainer(props=cluster_conf)"
    APC-->>CI: cluster_props_new
    CI->>CI: "platform.load_cluster_by_prop(cluster_props_new, is_inferred=True)"
Loading

Comments Outside Diff (1)

  1. user_tools/src/spark_rapids_pytools/cloud_api/dataproc.py, line 29 (link)

    P2 Remaining deprecated JSONPropertiesContainer usages not addressed by this PR

    The import of JSONPropertiesContainer is still required because three call-sites inside this file were not migrated: pricing_config construction (around line 118), raw_instances_descriptions (around line 323), and the _set_zone_from_props type annotation (line 531). All three will continue to emit the DeprecationWarning that this PR is trying to silence. The existing tests only cover the two paths mentioned in [BUG] Fix DeprecationWarning: Use AbstractPropContainer instead. This class does not support CspPaths. #1898, so the remaining warnings go undetected by the new test suite.

Reviews (1): Last reviewed commit: "fix(user_tools): replace deprecated prop..." | Re-trigger Greptile

Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
@greptile-apps

greptile-apps Bot commented Jun 23, 2026

Copy link
Copy Markdown

Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance.

…-prop-container

Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
@parthosa

Copy link
Copy Markdown
Collaborator

One JSON string path still needs to be handled. generate_cluster_configuration() returns a rendered JSON string, while AbstractPropContainer stores it without parsing. This causes cluster inference to fail with string indices must be integers (same reason as premerge failure).

Could you parse cluster_conf before constructing AbstractPropContainer? The test should also have generate_cluster_configuration() return json.dumps instead of the dictionary

Signed-off-by: WilliamK112 <164879897+WilliamK112@users.noreply.github.com>
@WilliamK112

Copy link
Copy Markdown
Author

Addressed in 1a91b80. ClusterInference.infer_cluster() now parses the rendered JSON string with json.loads() before constructing AbstractPropContainer. The regression test now has generate_cluster_configuration() return json.dumps(...) and asserts that nested inferred-cluster values remain accessible.

Validation:

  • PYTHONPATH=src .venv/bin/python -m pytest -q tests/spark_rapids_tools_ut/test_deprecated_prop_container.py (4 passed)
  • Pylint on both modified files (10.00/10)
  • Flake8 on both modified files
  • git diff --check

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

Labels

user_tools Scope the wrapper module running CSP, QualX, and reports (python)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Fix DeprecationWarning: Use AbstractPropContainer instead. This class does not support CspPaths.

3 participants