Backport 1.x - initial backport - #653
Merged
Merged
Conversation
These script are intended to be run as tests, but there is no visible information whether scripts were actually executed. Wrapping them up in print statements gives us confidence that scripts were run from start to finish. (cherry picked from commit 6683da9)
There is no plan to do anything with matlab bindings (we are not sure if we won't drop them whatsoever), but it would be useful to know if we accidentally break something in the future. Github actions allow us to use matlab. While originally intended way of running tests with ctests doesn't work for some reason (probably due to matlab setup in github actions), we still can run scripts there directly as commands. (cherry picked from commit d544408)
6e84e4d changed the way tests were written, but doing so dropped separation between c.segy and c++ tests. By bringing it back we remove duplicated test runs (reducing cases run in test entities 1-4 from 101 to 81, by 20 tests that belong to the c++ group and are not affected by mmap/lsb distinction). Note that c++ code is built and tests are run regardless of the "experimental" build option. That option impacts installation only. (cherry picked from commit bf3cfa6)
(cherry picked from commit f44aafb)
(cherry picked from commit 7e6fc9f)
(cherry picked from commit 9bed32d)
(cherry picked from commit d8f8b2b)
(cherry picked from commit 2d6ae3b)
(cherry picked from commit 64ce46a)
Two documentation strings in python/segyio/tools.py referred to SegyFile. This caused a problem with the tests, so I changed it to segyio.SegyFile. (cherry picked from commit 5bf27cc)
(cherry picked from commit 05bbd63)
Ubuntu-20.04 is deprecated and will be removed soon. (cherry picked from commit 62644e3)
Instead of deleter being function pointer, use custom type. In addition to getting rid of the warning, this is a preferred approach in defining custom deleters. (cherry picked from commit 5c69fb5)
(cherry picked from commit 9635261)
cppcheck never really worked. clang used together with scan-build was suspicious as compiler on compilation didn't identify as clang. (cherry picked from commit b3db880)
Removed duplicated job and added build on non-arm macos (cherry picked from commit a0d1703)
CMAKE_COMPILE_WARNING_AS_ERROR=ON is set to on. Reason is that with new coming adjustments for SEG-Y 2.1 types will change and we will need to be careful to do correct type conversions everywhere. Having failing jobs that should notice some type conversion mismatches should help. -D_CRT_SECURE_NO_WARNINGS is added to Windows jobs to prevent "security" warnings. Fixing these warnings properly would require different code paths for Linux-Windows and may have unexpected side-effects. As we deal with memory security in other ways, adding this flag is good enough. (cherry picked from commit 22592d9)
(cherry picked from commit 04da50b)
Conversions are now made explicit, no check over the conversion logic was performed. Anyway new logic would have to be applied with expansion to 8 byte numbers. (cherry picked from commit 6ce46fa)
Should allow us to make sure future development is not causing major drop in calculation speed. Currently only most straightforward cases are tested. Threshold is set to 25% of allowed slowness, which should be around x2 observed worst differences between two runs on the same sha. File sizes are reduced significantly to 1) fit into github actions runner 2) to assure stable enough environment in order to reduce speed fluctuations. Currently actions is also set to run on pull requests. We will see how often that fails and adjust triggers accordingly. Tests are considered to be secondary to main pytest suite and won't be run by default. Thus pytest-benchmark dependency is not considered to be a requirement for build and is not mentioned in the Readme. (cherry picked from commit ece8dc4)
We build and test wheels with cibuildwheel, but as no emulation happens there, tests are not actually run on bigendian system. In this new workflow we use qemu to emulate the setup. Run time is long, but we want this workflow anyway as our intention is to support bigendian environments. The most time-consuming operation is building numpy for s390x. Thus we run job only on merge to main as job is not expected to be failing often anyway. Alternatively we could build and cache numpy in github actions, but we decided to go with simpler solution for the moment. (cherry picked from commit cd874f4)
That fixes the only failure we have on bigendian environment (cherry picked from commit 8de6b93)
According to documentation, int24 test type is "quite incomplete". (See more information on original file creation at 6fe177c) Unfortunately that leads to confusion when trying to comprehend test behavior. This commit attempts to alleviate some of the confusion. 1. Intention of multiformat tests is to assure that files in all formats are read correctly. To test it f3.sgy file (format 3) was converted to all other formats. In theory we would like to assure that each sample value x in format n equals corresponding sample value in format 3. Instead we verify not that values are the same, but just that some bytes are. (Examples below are laid out as bigendian). 1a: From 2-byte signed integer f3.sgy file, 3-byte signed integer files were created by setting the most significant byte to 0. While numbers 0x12 0x34 (4660 dec) and 0x00 0x12 0x34 (4660 dec) are equivalent when read as signed 2-byte 2's complement int and signed 3-byte 2's complement int, 0xAB 0xCD (-21555 dec) and 0x00 0xAB 0xCD (43981 dec) are not. In the code casting from f3 format (int16) to int24 is implemented such that most significant byte is always set to 0. Thus f3 value, regardless of sign, is transformed to the same value that was written into format7 file. Behavior was intended, but is confusing, especially because 4-byte and 8-byte signed/unsigned int files are sign-aware. To improve this, int24 must be made sign-aware and format 7/15 files must be fixed by setting 0xFF instead of 0x00 byte for negative numbers - turn 0x00 0xAB 0xCD (43981 dec) to 0xFF 0xAB 0xCD (-21555 dec). This is fixed with a one-time script equivalent of for i in range(samples_value): bytes_read = f.read(3) if bytes_read[1] & 0x80: f.seek(-3, os.SEEK_CUR) if bigendian: f.write(b'\xFF' + bytes_read[1:]) else: f.write(bytes_read[:2] + b'\xFF') 1b: Similar value inequality happens for 1-byte formats 8 and 16 also. 2-byte integer file was converted to 1-byte integer files by cutting of the most significant byte. Usually values in f3.sgy do not have most significant byte = 0, so 2-byte int value 0x12 0x34 in f3 became 0x34. 0x12 0x34 is not equal 0x34, so values in the files are not equal. However comparison is done by casting f3 values into 1-byte type value, which by default will cut off the most significant byte. So in this case only the least significant bytes are compared and thus test passes. 1c: As original file contains signed integers, they are impossible to correctly represent as unsigned integers. Thus rules are defined by conversion of signed type to unsigned. So for example signed 2-byte int 0xF5 0xCE (-2610) and unsigned 4-byte int 0xFF 0xFF 0xF5 0xCE (4294964686) are considered equal. A cleaner approach would require creation of valid 1-byte files and their conversion to different formats, but it would require effort, which currently is not warranted as existing solution is good enough. 2. conversion to int32 is only used during comparison, so it could be implemented any way that can state whether internal bytes are the same or not. So while not required by tests, implementation is changed to actually represent the number that was provided. (cherry picked from commit bc7ac72)
Current logic is flawed. Intention of benchmark tests was to build library from two different commits. However only C/C++ library was built separately, python code was not touched and included. When current commit's directory was checked out to run the fresh tests, so was other python code: tests were always run on the same python part of the library. To fix that, build a local wheel from existing code and make sure it is being used instead of local code. (cherry picked from commit 3ca438f)
Red Hat Enterprise Linux (RHEL) 7 has reached its end of life in 2024. This means we can bump the minimum required version to 3.11 which is provided in RHEL 8 [1]. We assume that other systems either have CMake 3.11 or newer available, or have a way to install a sufficently new CMake versions. Bumping the minimum version removes some deprecation warning (about support for older CMake versions being dropped), and also ensures compatibility with CMake 4.0 which has dropped support for CMake <3.5 and was rolled out to GitHub runners recently. [1]: https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/8/html-single/8.0_release_notes/index#platform-tools (cherry picked from commit 3e2cbe6)
- `algorithm` must be included because the header uses functions like `copy_n`. - `cstdint` must be included explicitly on Fedora-based Linux distribtuions, like Red Hat Enterprise Linux 8, to make all the used integer types available. - `stdexcept` must be included for definition of exceptions such as `runtime_error`. (cherry picked from commit abfa92c)
These tests try to check whether the inclusion of segyio headers that are part of segyio's public interface prevent other projects from compiling. We had cases in which our public facing C and C++ headers did not define all required includes. This could lead to failures depending on the compiler and what other includes were and in which order when a segyio header was included. Testing for the correct headers to be included appears to be non-trivial and it is not 100% clear if this test is sufficiently accurate to prevent further problems in the future. At least the test tries to approximate how segyio's C/C++ interfaces are included in other projects such as the the si4ti project [1] that uses the C++ interface. If our tests is too simple, we could also try a tool like "include-what-you-use" [2]. If our test does not provide sufficient value, we can remove it again in the future. Note, the added test should fail at compile time if anything is wrong with the includes. [1]: https://github.com/equinor/si4ti [2]: https://github.com/include-what-you-use/include-what-you-use (cherry picked from commit c35e43d)
That speeds up Github Actions workflow from around 35 minutes to 7.5 minutes. We are now fast enough to run this job on pull requests. Numpy is a bit oldish, but it shouldn't matter to us. (cherry picked from commit dec89ae)
Caching environment should provide additional speedup in certain cases. If cache is available, job could finish in 4 minutes. (cherry picked from commit 662f147)
The `collect` example was depicting to reshape based on `f.samples` - an array, instead of `len(f.samples)` (cherry picked from commit 09d835d)
This allows us to use CMake's new way of finding and interacting with Python. This also acts as preparatory step to move to the build process of the Python extension from `scikit-build` to `scikit-build-core` [1] which requires CMake >=3.15. CMake 3.18 is chosen as minimum version because the usage of the `WITH_SOABI` option is recommended [2] which requires CMake >=3.17 [3] and allows to use `Development.Module` (introduced in CMake 3.18) instead of the more generic `Development` component. [1]: https://scikit-build-core.readthedocs.io/en/latest/ [2]: https://scikit-build-core.readthedocs.io/en/latest/guide/migration_guide.html [3]: https://cmake.org/cmake/help/latest/module/FindPython.html#commands (cherry picked from commit acd3b82)
This options did not have any observable effect because we search for the Python packages with ``` find_package(Python COMPONENTS Interpreter REQUIRED) ``` which will issue a fatal error if it cannot find the specified PYthon components. This means that we would never reach our own error handling if the Python interpreter was missing. Instead, we can rely on the `BUILD_PYTHON` build option to decide whether we want to include the Python-specific build path or not. (cherry picked from commit 87436ac)
We now choose the the same OSX deployment target for the segyio core library and its Python extension when building the wheels for MacOS. This fixes warnings that we can observe in the GitHub Actions: ``` ld: warning: object file (/usr/local/lib/libsegyio.a[2](segy.c.o)) was built for newer 'macOS' version (14.0) than being linked (11.0) ``` (cherry picked from commit 3182a86)
We were experiencing some issues with scikit-build on the GitHub runners for Windows builds while also facing some deprecation warnings w.r.t. the build system. This is solved by moving to scikit-build-core for building the Python extension. scikit-build-core is currently more actively developed and more modern replacement for scikit-build for projects that do not need to manipulate the build process excessively. The changes mostly follow the migration guide [1]. Changing to scikit-build-core also allows us to simplify the the build process of the Python extension. The `Manifest.in` for building a source distribution (sdist) was removed since it does not have any use anymore. We do not build a source distribution at the moment. If we want to build one again, we need to configure it inside the `pyproject.toml`. [1]: https://scikit-build-core.readthedocs.io/en/latest/guide/migration_guide.html UPD achaikou: backport-1.x branch: cherry pick required updates to setup.cfg file. Originally cherry picked from commit f01a503
A workaround to make the tests pass on current GitHub runners for Windows is not needed anymore after the move to scikit-build-learn. (cherry picked from commit 58a2485)
Removed explicitly skipped builds that are either not selected (pp-*) nor supported (cp36 and cp37) by cibuildwheel 3.0. This removes warning issued by cibuildwheel in the corresponding GitHub Action. (cherry picked from commit 796b42a)
There is no obvious reason anymore to restrict the base image to manylinux2014. Additionally, the usage of the manylinux2014 negatively impacts the time it takes for cibuildwheel to test the wheels on Python >=3.11 because NumPy must be build from source in this case. (cherry picked from commit 858f455)
(cherry picked from commit d9bbc01)
Disable warnings for methods exposed to Python arguments and potentially missing field initializers in Python-specific data structures. Removing the cause of the warnings is, in parts, impossible, or tedious as it depends on the Python version the Python bindings are compiled for. Note: The warnings became visible because we updated the build process of the Python bindings in a previous commit. Earlier, additional compiler flags for warnings and making warnings into errors were not forwarded to the Python binding's build process but are forwarded now. **Methods exposed to Python (-Wcast-function-type)** The PyMethodDef always expects a pointer to a C implementation of type `PyCFunction` as second argument. `PyCFunction` expects two input arguments. If a method has only one argument, e.g., a method that only accepts `self`, or a method with more than two arguments, e.g., a Python method with keyword arguments, the compiler issues a warning. As far as I understand, this cannot be resolved for functions with keyword arguments (`METH_VARARGS | METH_KEYWORDS`, see [1]) that have three input arguments. We can either cast the method to `PyCFunctionWithKeywords` and get an error about too many input arguments or we cast to PyCFunction and get an error about non-matching function types. **Missing field initalizers (-Wmissing-field-initializers)** The number of fields in structs defined by the Python interface depend on the version number. To fix this we would need, potentialy, confusing `#if ... #else` constructions in the code. **Guarding of pragmas** The used pragmas are not standardised but depend on the compilers. Therefore, we use pragmas specific to the currently detected compiler to avoid errors about unknown pragmas [1]: https://docs.python.org/3/c-api/structures.html [2]: https://stackoverflow.com/a/10264563 UPD achaikou: backport-1.x branch: cherry pick required solving merge conflicts. Originally cherry picked from commit 8c0102a
Contributor
|
To me to the changes look good overall. I also don't know if there is any better way to keep a branch for segyio v1 than the way that you propose it here. Also for understanding/alignment. I assume the workflow in the future for the
Is this correct? RemarksGeneral
List of commitsI went through the commit history of Before cutoffOctober 16d127d60 Add additional tests to CI that build Python bindings June 6f041398 Fix printf in segyio-catb May 713f0be1 Add CPP check for macos 13 After cutoff (too new)I add these for completeness since I went through the full history. I am completely fine to add these later. October 297bebfd7 Reduce permissions of GitHub tokens in Actions October 2809fa7f3 Improve return value check October 227b4470c Declare C++11 as used in whole project |
The tests are used to verify that the Python bindings do not emit any unexpected warnings. (cherry picked from commit d127d60)
Add explicit EOF check in segyio-catr when reading from command line options. (cherry picked from commit 09fa7f3)
(cherry picked from commit 7bebfd7)
(cherry picked from commit 466cf21)
This commit updates the publication workflow for Trusted Publishing via GitHub Actions [1]. Further configuration changes are made on the repository (GitHub) and PyPI level [2]. [1]: https://packaging.python.org/en/latest/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/ [2]: https://packaging.python.org/en/latest/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/#configuring-trusted-publishing (cherry picked from commit 7f5b253)
Python 3.14 changed how object reference counts work [1], often resulting in lower counts than before. In RefTrace, this reduces the internal trace reference count from 3 to 2 for objects that may be garbage collected. [1]: https://docs.python.org/3/whatsnew/3.14.html#optimizations (cherry picked from commit 7fc9868)
(cherry picked from commit 252acc4)
achaikou
force-pushed
the
backport-1.x_populate
branch
from
November 28, 2025 18:33
d31c792 to
ba82fe6
Compare
achaikou
commented
Nov 28, 2025
achaikou
left a comment
achaikou
left a comment
Contributor
Author
There was a problem hiding this comment.
Yes, I also expect workflow like that.
May 7
13f0be1 and c0c9ef5 - not added. Check itself could be ok, but fix commit relies on 2.0 changes
June 6
f041398 - not added. This is fix for functionality added in 2.0
October 16
d127d60 - added, though I don't think we must follow workflow changes very closely.
October 22
7b4470c - not added. The commit came when we bumpted segyio.cpp to c++11, which we don't do here.
October 28
09fa7f3 - ok, can be
October 29
7bebfd7 - yes, added
Also added:
October 28: Renovate badges
October 28: Update Action for Trusted Publishing on PyPI
November 14: Update RefTrace's garbage collection
November 17: Enable Python 3.14 build
And I added those too:
- 3.9 Python drop
- SEG-Y links change
maybe will add a cherry-pick comment to them later on rebase.
Or they will stay duplicated like that
Regarding note to Readme:
it probably should go to the main branch, not just to backport one?
Also not sure if changelog should be done both on main + backport or only on backport.
At least version change should happen only here directly.
Anyway that is a separate PR, probably.
Also I still don't like the backport name for the branch. 😒
Maybe
segyio-1.x
?
Also weirdly cherry-pick references in commits for some reason seem to point to my repo, but I hope it will change once PR is merged...
ajaust
approved these changes
Dec 2, 2025
ajaust
left a comment
ajaust
left a comment
Contributor
There was a problem hiding this comment.
Looks good overall. I just added some two small comments to the commits.
Below, replies to your comment. 🙂
Workflow 👍
Picked commits 👍
Note to README:
Yes, I meant that we should add a note to the README.md in the main branch. The intention is to make it visible to users where the v1 code is located.
Changelog:
I think we should have an updated changelog in both branches since we plan to have a release for both branches as well. This makes it simple for users to get a quick overview of the changes changes depending on the release.
I agree, making a release/adding a changelog is another PR.
Yes, I like segyio-1.x as branch name. Just update the GitHub Actions accordingly
I don't know what type of references are generated by the cherry-pick. I have only checked the commits on GitHub and there does not seem any reference to your fork or I did not see it.
achaikou
force-pushed
the
backport-1.x_populate
branch
from
December 2, 2025 09:53
ba82fe6 to
3a65a38
Compare
Contributor
|
I marked the last open comments as resolved. 🙂 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes gathering cut-off is commit 1e7a22d
First PR against
backport-1.xsegyio-1.xbranch. Branch name can be up for discussion, if we want something nicer here, as it is not just backport but 1.0 release. I already vetoed namev1.xas it is similar to tags andrelease-1.xas it is similar to one existing branch which might be used for something).Having the separation done already now is sponsored by macos intel starting to break in November.
I don't know how backporting stuff is done properly and if it works the way I think it works, so lets figure it out.
PR Contains general commits from "main" branch. I tried to add there commits touching only workflows-tests-docs, probably not all of them anyway, but major ones. But no functional changes.
Publishing workflow is still a bit of a mystery, though looks like nothing specific would be required. We might need to test it on test-pypi before publishing 3.14 python release.
We probably don't need benchmarks workflow, but I picked commits with it for simplicity. Also it might make it a bit easier to switch base benchmarking reference to next segyio 1.0 release later?..
Btw, this PR truly shows the power of atomic commits 😄
I could cherry-pick commits from the last 6 months and get like two small merge conflicts 😃
The further we would go, the worse problem would obviously become, but probably if that happens,
backport-1.xbranch should have only python version updates and nothing more.See if you think I am missing some commits/added some commits that shouldn't be here/should have done this all differently.