Closed praneethratna closed 10 months ago
Merging #1191 (ba38789) into dev (ce1e614) will increase coverage by
0.06%
. Report is 1 commits behind head on dev. The diff coverage isn/a
.
@@ Coverage Diff @@
## dev #1191 +/- ##
==========================================
+ Coverage 81.89% 81.95% +0.06%
==========================================
Files 67 67
Lines 5887 5908 +21
==========================================
+ Hits 4821 4842 +21
Misses 1066 1066
Flag | Coverage Δ | |
---|---|---|
unittests | 81.95% <ø> (+0.06%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
see 2 files with indirect coverage changes
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
Thanks so much for this PR and for the links! Great detective work. charset-normalizer
is clearly the source of the problem.
The current version of charset-normalizer
is 3.3.0. This is the version that was being installed last week. In fact, that version was released on 9-30, so there is a good chance that that version is the one that actually caused the problem.
The discussions you linked to are from early this year, and they recommend 3.1.0 because at the time that was the latest and the problems being reported were based on older versions.
In general, whenever possible, I think it's best to avoid pinning to a specific version. My recommendation is to change your addition to charset-normalizer<3.3
Hmm. charset-normalizer
3.2.0 was installed in the CI, as expected. 3.2.0 was released on June 7, and was the dependency version we likely used successfully for a little less than 4 months.
But now the CI is failing again, with the same error. It would seem charset-normalizer
3.1.0 does solve the problem (though there were some hiccups in the CI), but 3.2.0 doesn't. That doesn't make sense to me, given our recent experience. I would hold off on merging this PR until we have a better handle of this.
BTW, there's a pip installation error in the CI log involving the botocore
version that was installed -- but only for Py3.11!
I'm pasting some exchanges @lsetiawan and I had on Friday about this. I was going to create a new issue about this.
EMILIO: Starting yesterday we are running into a puzzling error with the CI tests and possibly a linked error when the test suite is run locally. In both cases, the error happens with tests/convert/test_parsed_to_zarr.py
, but they play out differently. In the CI, it's a bigger, low level problem spawned by aiohttp
:
ImportError while importing test module '/home/runner/work/echopype/echopype/echopype/tests/convert/test_parsed_to_zarr.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../../../micromamba/envs/echopype/lib/python3.10/site-packages/aiohttp/client_reqrep.py:70: in <module>
import cchardet as chardet
E ModuleNotFoundError: No module named 'cchardet'
In my local tests with a freshly created Py 3.10 development environment, the problem is limited to a subset (3 instances) of the tests/convert/test_parsed_to_zarr.py::TestParsed2Zarr::test__create_zarr_info
tests, and it's spawned by botocore
instead. Here are snippets of the error report:
ERROR echopype/tests/convert/test_parsed_to_zarr.py::TestParsed2Zarr::test__create_zarr_info[s3://echopype-test/my-dir/] - botocore.exceptions.ClientError: An error occurred (IllegalLocationConstraintException) when calling the CreateBucket operation: The unspecified location constraint is incompatible for the region specific endpoint this request was sent to.
/home/mayorga/miniconda3/envs/echopype_dev/lib/python3.10/site-packages/botocore/client.py:980: ClientError
We're seeing this in PRs #1182 and #1189. Here's the error start from PR #1182. All 3 Python versions are failing, but I've only examined the Py 3.10 log report.
DON: That's interesting... I have not seen that before! I'm currently working a lot on parsed to zarr stuff and those tests will actually be removed. So I'm wouldn't investigate too far on that. Probably best to ignore for now!
EMILIO: Actually, looking at the CI log again, I'm realizing that there are other errors as well. The "ModuleNotFoundError: No module named 'cchardet'" spawned by aiohttp is also taking place, in convert/test_convert_source_target_locs.py
rather than test_parsed_to_zarr.py
. ... the issue may extend beyond the stuff you're deprecating.
DON: "ModuleNotFoundError: No module named 'cchardet'"": This make me suspect a bump in dependency somewhere...
But now the CI is failing again, with the same error. It would seem
charset-normalizer
3.1.0 does solve the problem (though there were some hiccups in the CI), but 3.2.0 doesn't. That doesn't make sense to me, given our recent experience. I would hold off on merging this PR until we have a better handle of this.
I think some of dependencies require cchartdet
and some need charset-normalizer
which is causing circular imports in charset-normalizer
>3.1 versions and that might be due to an update to dependency that requires cchardet
. Also, the cchardet
module doesn't support python3.11 due to which adding cchardet
to requirements.txt
is causing an error during installation of libraries in python3.11.
BTW, there's a pip installation error in the CI log involving the
botocore
version that was installed -- but only for Py3.11!
I think this might be a one time issue since when i re ran the tests, that error did not occur again.
Hi @praneethratna! Thank you so much for this very detailed investigation. This is such great detective work. After looking through the CI, I've narrowed down the problem to version mismatch during the CI setup. Currently this system is quite convoluted as we're installing pip packages and conda packages causing mismatch on dependencies and the charset-normalizer
being installed twice with the conda package being 3.3.0
and the pip package is 3.2.0
. Your pinning above did work, which was really great. However, this would introduce a pinned dependency to an upstream package that we don't directly use. Having this pinning could potentially cause other dependency resolving issues in the future, so instead I've removed the conda piece in PR https://github.com/OSOceanAcoustics/echopype/pull/1192. This has been something in the back burner and looks like this is time to make those changes. Now that the package is using a single package manager, all looks good and I'm closing this PR. Thanks again!
Fixed failing CI tests which is caused due to
charset-normalizer
dependency version error for which version should be explicitly set to3.1.0
.Discussions regarding the error and its fix - Stackoverflow, Github.
CC @emiliom