Skip to content

Replace cwrap.BaseCClass - #1286

Merged
eivindjahren merged 14 commits into
mainfrom
remove_base_c_class
Sep 11, 2026
Merged

Replace cwrap.BaseCClass#1286
eivindjahren merged 14 commits into
mainfrom
remove_base_c_class

Conversation

@eivindjahren

@eivindjahren eivindjahren commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

First does some minor clean up in the c code, then replaces cwrap.BaseCClass

@eivindjahren
eivindjahren changed the base branch from remove_copen to remove_custom_arrays September 1, 2026 07:10
@eivindjahren eivindjahren changed the title Remove base c class Remove BaseCClass Sep 3, 2026
@eivindjahren eivindjahren changed the title Remove BaseCClass Replace cwrap.BaseCClass Sep 3, 2026
@eivindjahren
eivindjahren force-pushed the remove_base_c_class branch 9 times, most recently from 04e5b18 to 758d73c Compare September 3, 2026 13:14
@eivindjahren
eivindjahren requested a lite review from Copilot September 3, 2026 13:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new _BaseCClass lacks an automatic cleanup path for owning wrappers and FortIO::fwrite_error() can now throw during error handling, both of which can cause runtime issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR removes the external Python dependency on cwrap by introducing an internal _BaseCClass and migrating the Python wrapper classes (Summary/Grid/ResdataKW/etc.) off cwrap.BaseCClass, alongside some C++/header cleanup (removing util_unlink, switching some headers to #pragma once).

Changes:

  • Drop cwrap from Python runtime dependencies and update tests/imports accordingly.
  • Introduce python/resdata/_base_c_class.py and migrate multiple Python wrapper classes from BaseCClass to _BaseCClass; migrate SummaryVarType from BaseCEnum to IntEnum.
  • Remove util_unlink and replace unlink-on-write-error behavior with std::filesystem operations; modernize several headers to #pragma once.
File summaries
File Description
uv.lock Removes cwrap from locked dependency metadata.
tests/rd_tests/test_sum.py Drops unused cwrap import.
tests/rd_tests/test_grdecl.py Drops unused cwrap import.
tests/rd_tests/test_fault_blocks.py Drops unused cwrap import.
setup.py Removes cwrap from install_requires.
python/resdata/summary/rd_sum.py Switches Summary to inherit _BaseCClass.
python/resdata/summary/rd_sum_var_type.py Replaces BaseCEnum with IntEnum.
python/resdata/summary/rd_sum_tstep.py Switches SummaryTStep to inherit _BaseCClass.
python/resdata/summary/rd_sum_keyword_vector.py Switches to _BaseCClass and adds __iter__.
python/resdata/resfile/rd_kw.py Switches ResdataKW to inherit _BaseCClass and adjusts typing.
python/resdata/rd_type.py Switches ResDataType to inherit _BaseCClass.
python/resdata/grid/rd_region.py Switches ResdataRegion to inherit _BaseCClass.
python/resdata/grid/rd_grid.py Switches Grid to inherit _BaseCClass.
python/resdata/grid/faults/layer.py Switches Layer to inherit _BaseCClass.
python/resdata/grid/faults/fault_block_layer.py Switches FaultBlockLayer to inherit _BaseCClass.
python/resdata/gravimetry/rd_subsidence.py Switches ResdataSubsidence to inherit _BaseCClass.
python/resdata/gravimetry/rd_grav.py Switches ResdataGrav to inherit _BaseCClass.
python/resdata/geometry/surface.py Switches Surface to inherit _BaseCClass.
python/resdata/geometry/geo_region.py Switches GeoRegion to inherit _BaseCClass.
python/resdata/geometry/geo_pointset.py Switches GeoPointset to inherit _BaseCClass.
python/resdata/geometry/cpolyline_collection.py Switches CPolylineCollection to inherit _BaseCClass.
python/resdata/_base_c_class.py New internal replacement for cwrap.BaseCClass.
lib/util/util_unlink.cpp Deletes the platform-specific unlink shim implementation.
lib/resdata/FortIO.cpp Replaces unlink-on-write-error with std::filesystem::remove and tightens file handling.
lib/include/resdata/rd_units.hpp Converts header guard to #pragma once and removes C-linkage wrapper.
lib/include/resdata/rd_kw_magic.hpp Converts header guard to #pragma once and removes C-linkage wrapper.
lib/include/resdata/rd_grav_common.hpp Converts header guard to #pragma once and removes C-linkage wrapper.
lib/include/resdata/rd_endian_flip.hpp Converts header guard to #pragma once and removes C-linkage wrapper.
lib/include/ert/util/util.hpp Removes inclusion of deleted util_unlink.hpp.
lib/include/ert/util/util_unlink.hpp Deletes the unlink shim header.
lib/include/ert/util/test_work_area.hpp Converts header guard to #pragma once and removes C-linkage wrapper.
lib/include/ert/util/test_util.hpp Converts header guard to #pragma once and adjusts linkage for util_abort_test_jump_buffer().
lib/CMakeLists.txt Removes util_unlink.cpp from the build.
Review details

Suppressed comments (1)

setup.py:146

  • This PR removes cwrap from runtime dependencies here, but pyproject.toml still lists cwrap under [dependency-groups].build, which will keep pulling it into common dev/build environments. If cwrap is no longer needed, it should be removed there as well to fully complete the dependency cleanup.
    install_requires=[
        "numpy",
        "pandas",
        "python-dateutil",
        "natsort",
        "typing_extensions",
        "resfo-utilities>=0.4.0",
    ],
  • Files reviewed: 32/33 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/resdata/FortIO.cpp
Comment thread python/resdata/_base_c_class.py
Comment thread python/resdata/summary/rd_sum_keyword_vector.py Outdated
Base automatically changed from remove_custom_arrays to main September 4, 2026 08:53

@ajaust ajaust left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice work.

I seem to have mostly the same comments and questions as Copilot.

Comment thread lib/resdata/FortIO.cpp
Comment thread python/resdata/_base_c_class.py
Comment thread python/resdata/_base_c_class.py
Comment thread python/resdata/summary/rd_sum_keyword_vector.py Outdated
Comment thread python/resdata/_base_c_class.py
@eivindjahren
eivindjahren force-pushed the remove_base_c_class branch 2 times, most recently from 13c012a to 80d999f Compare September 10, 2026 13:30
@eivindjahren
eivindjahren requested review from ajaust and a lite review from Copilot September 10, 2026 13:31

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

It changes cross-language binding semantics and native ownership/representation broadly, which needs careful human validation beyond automated review.

Review details
  • Files reviewed: 54/55 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread lib/resdata/FortIO.cpp
Comment thread lib/resdata/rd_grav.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The broad cross-language pointer ownership and native lifetime changes warrant final human validation.

Review details
  • Files reviewed: 54/55 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

It introduces an unresolved public ABI linkage break and leaves the cwrap build dependency in place.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 54/55 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread lib/include/resdata/rd_grav_common.hpp
Comment thread python/resdata/_base_c_class.py
@eivindjahren
eivindjahren force-pushed the remove_base_c_class branch 2 times, most recently from 27e4c74 to d8682a5 Compare September 11, 2026 06:16
@eivindjahren
eivindjahren requested a balanced review from Copilot September 11, 2026 06:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The extensive native pointer and ownership migration needs human validation, and dependency/documentation cleanup remains incomplete.

Review details
  • Files reviewed: 56/57 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread python/resdata/resfile/rd_kw.py
Comment thread python/resdata/_base_c_class.py
This is rather than boxing the address as a Python int.

_BaseCClass stores the pointer it receives as its "_BaseCClass__c_pointer"
attribute. Now a capsule holds the address as an actual void* struct field, so
unlike a Python int (which represents arbitrary precision integers as an
array of 30-bit digits, never as a single contiguous machine word) it
remains visible to memory scanners such as LeakSanitizer. A
null pointer is represented as None, so need to consistently use
"is None"/"is not None" rather than check __bool__ of PyCapsule.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

It changes native pointer representation and ownership across numerous Python and C++ interfaces.

Review details
  • Files reviewed: 58/59 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@eivindjahren
eivindjahren merged commit 2eae57d into main Sep 11, 2026
12 checks passed
@eivindjahren
eivindjahren deleted the remove_base_c_class branch September 11, 2026 07:17
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.

3 participants