Skip to content

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Aug 3, 2025

Rationale for this change

Looks like something changed, which caused the CI to fail: https://github.com/apache/iceberg-python/commits/main/

First attempt to isolate the issue (checking if it is related to coverage)

Are these changes tested?

Are there any user-facing changes?

@kevinjqliu
Copy link
Contributor

weird that the test fails with

tests/avro/test_reader.py:24: in <module>
    from pyiceberg.avro.decoder_fast import CythonBinaryDecoder
E   ModuleNotFoundError: No module named 'pyiceberg.avro.decoder_fast'

but in the CI's Install step, it shows decoder_fast being compiled

Run make install-dependencies
poetry install --all-extras
Installing dependencies from lock file

No dependencies to install or update

Installing the current project: pyiceberg (0.10.0)
Preparing build environment with build-system requirements poetry-core>=1.0.0, wheel, Cython>=3.0.0, setuptools
Compiling pyiceberg/avro/decoder_fast.pyx because it changed.
[1/1] Cythonizing pyiceberg/avro/decoder_fast.pyx

@Fokko
Copy link
Contributor Author

Fokko commented Aug 3, 2025

Yes, it looks like it isn't being added to the path for some reason. I'm not sure where this comes from, but I suspect some dependency that's not correctly pinned.

I noticed that we have some build dependencies without a specific version:

[build-system]
requires = ["poetry-core>=1.0.0", "wheel", "Cython>=3.0.0", "setuptools"]
build-backend = "poetry.core.masonry.api"

But non of them had a release recently, so it could also be a downstream dependency somewhere that doesn't isn't correctly pinned. Or it could be a red-herring, the integration tests are working fine. It seems to be an issue when it is imported from the tests/ folder. Anyway, another reason to rip out the Cython part 😁

@kevinjqliu
Copy link
Contributor

i think we're using the same prompt haha

Check if .so file exists

Run ls -lh pyiceberg/avro/
total 1.4M
-rw-r--r-- 1 runner docker 1021 Aug  3 20:56 __init__.py
drwxr-xr-x 2 runner docker 4.0K Aug  3 20:56 codecs
-rw-r--r-- 1 runner docker 5.9K Aug  3 20:56 decoder.py
-rw-r--r-- 1 runner docker 2.0K Aug  3 20:56 decoder_basic.c
-rw-r--r-- 1 runner docker 619K Aug  3 20:57 decoder_fast.c
-rw-r--r-- 1 runner docker 521K Aug  3 20:58 decoder_fast.cpython-310-x86_64-linux-gnu.so
-rw-r--r-- 1 runner docker 178K Aug  3 20:57 decoder_fast.html
-rw-r--r-- 1 runner docker 1.8K Aug  3 20:56 decoder_fast.pyi
-rw-r--r-- 1 runner docker 6.3K Aug  3 20:56 decoder_fast.pyx
-rw-r--r-- 1 runner docker 3.0K Aug  3 20:56 encoder.py
-rw-r--r-- 1 runner docker  11K Aug  3 20:56 file.py
-rw-r--r-- 1 runner docker  17K Aug  3 20:56 reader.py
-rw-r--r-- 1 runner docker  21K Aug  3 20:56 resolver.py
-rw-r--r-- 1 runner docker 7.0K Aug  3 20:56 writer.py
Print PYTHONPATH and files

Run echo $PYTHONPATH

./build/temp.linux-x86_64-cpython-310/pyiceberg/avro/decoder_fast.o
./build/lib.linux-x86_64-cpython-310/pyiceberg/avro/decoder_fast.cpython-310-x86_64-linux-gnu.so
./.mypy_cache/3.10/pyiceberg/avro/decoder_fast.meta.json
./.mypy_cache/3.10/pyiceberg/avro/decoder_fast.data.json
./pyiceberg/avro/decoder_fast.pyx
./pyiceberg/avro/decoder_fast.c
./pyiceberg/avro/decoder_fast.cpython-310-x86_64-linux-gnu.so
./pyiceberg/avro/decoder_fast.pyi
./pyiceberg/avro/decoder_fast.html

from kevinjqliu#18

this test passed... maybe it was intermittent or just a bad host machine?

@Fokko
Copy link
Contributor Author

Fokko commented Aug 3, 2025

@kevinjqliu Hahaha nice!

this test passed... maybe it was intermittent or just a bad host machine?

I hope so 🤞 This is pretty annoying :D

@kevinjqliu
Copy link
Contributor

ok this is interesting.

the .so file is decoder_fast.cpython-310-x86_64-linux-gnu.so which implies its built for python 3.10

this PR, the test failed. and poetry is using 3.9

poetry run pytest tests/ -m "(unmarked or parametrize) and not integration" -v  
============================= test session starts ==============================
platform linux -- Python 3.9.23, pytest-7.4.4, pluggy-1.6.0 -- /home/runner/.cache/pypoetry/virtualenvs/pyiceberg-bKWKvoA4-py3.9/bin/python

my pr, the test passed. and poetry is using 3.10

platform linux -- Python 3.10.12, pytest-7.4.4, pluggy-1.6.0 -- /home/runner/.cache/pypoetry/virtualenvs/pyiceberg-bKWKvoA4-py3.9/bin/python

In fact, if you check all 4 lint-and-test and expand Run unit tests with coverage, they all say 3.10
https://github.com/kevinjqliu/iceberg-python/actions/runs/16709346308/job/47291940694?pr=18

🤔

@Fokko
Copy link
Contributor Author

Fokko commented Aug 3, 2025

Interesting, but not sure if anything changed there?

#2267 seems to pass as well, after disabling the cache. Also pushed an empty commit on #2265 to check if it has to do with the caching.

@Fokko
Copy link
Contributor Author

Fokko commented Aug 3, 2025

Maybe we should disable the caching for now? It looks like the difference in time is minimal.

With caching 3m39s: https://github.com/apache/iceberg-python/actions/runs/16532954029/job/46762141235
Without caching 3m48s (this PR): https://github.com/apache/iceberg-python/actions/runs/16709541251/job/47292439897?pr=2268

@Fokko Fokko marked this pull request as ready for review August 3, 2025 21:24
@Fokko Fokko changed the title Debug CI CI: Disable caching Aug 3, 2025
@kevinjqliu
Copy link
Contributor

nice catch, another workaround can be to manually force build-module.py by adding this step in the CI definition

poetry run python build-module.py build_ext --inplace

@Fokko
Copy link
Contributor Author

Fokko commented Aug 3, 2025

@kevinjqliu I'd rather get rid of the native code in this repository, instead of adding workarounds :) Thanks for looking into this 👍

@Fokko Fokko merged commit 0c84aaf into apache:main Aug 3, 2025
10 checks passed
@Fokko Fokko deleted the fd-debug-ci branch August 3, 2025 21:33
gabeiglio pushed a commit to Netflix/iceberg-python that referenced this pull request Aug 13, 2025
<!--
Thanks for opening a pull request!
-->

<!-- In the case this PR will resolve an issue, please replace
${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->

# Rationale for this change

Looks like something changed, which caused the CI to fail:
https://github.com/apache/iceberg-python/commits/main/

First attempt to isolate the issue (checking if it is related to
coverage)

# Are these changes tested?

# Are there any user-facing changes?

<!-- In the case of user-facing changes, please add the changelog label.
-->
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.

2 participants