vectorcode.chromadb: enable check phase#417811
Conversation
github-actions
bot
added
10.rebuild-darwin: 1-10
|
|
When running nixpkgs-review on Darwin: Please disable that test (preferably on Darwin alone) |
|
Thanks for building on Darwin! I only have x86_64-linux machines... I need to get around to setting up the GitHub action that runs nixpkgs-review on all platforms! Back on topic... Is there an easy way to tell the tests to run sequentially? E.g. some pytest flag? This looks like a concurrency issue. |
It's a known issue where |
|
I assume it is better to disable the entire Also, I assume we want to disable on EDIT: implemented it as above, for now |
MattSturgeon
force-pushed
the
chromadb_0/tests
branch
from
abb367b to
621dfce
Compare
Use |
Is this an issue for hydra too, or just nixpkgs-review? I'd prefer to have as many tests enabled as possible, but if this will cause disruption for the package maintainers then disabling is fine too. If it can cause problems on hydra, then it should be disabled; no question. |
It can cause problems on Hydra as well, depending on timing. Better to disable the file. |
|
Looks like this is the same concurrency issue on linux:
EDIT: could be related to this warning from pytest-asyncio: ? |
This comment was marked as outdated.
This comment was marked as outdated.
The error in the logs doesn't seem related the concurrency issue, which seems to have been fixed by When introduced by @GaetanLepage in #394565, vectorcode was marked "broken" on aarch64-linux. This seems to have been inherited from chromadb. It looks like #412528 (accidentally?) removed Should |
MattSturgeon
force-pushed
the
chromadb_0/tests
branch
from
cc5352f to
6e026a0
Compare
MattSturgeon
force-pushed
the
chromadb_0/tests
branch
from
6e026a0 to
61530e0
Compare
No and no. Test collection is breaking on aarch64-Linux, probably due to a missing package that's used in the tests. That doesn't mean the package is broken. ADDED: Getting the tests to work in 0.6.3 originally required a fair bit of work. Testing 1.0.x needed different supporting libraries and settings. I'm not surprised you're having test issues in certain conditions. You may want to peruse the diff in 6126501 again in case something was missed. |
|
Same failure with > =========================== short test summary info ============================
> FAILED chromadb/test/test_chroma.py::GetAPITest::test_local - ValueError: An instance of Chroma already exists for ephemeral with different settings
> !!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!!
> !!!!!!!!!!!! xdist.dsession.Interrupted: stopping after 1 failures !!!!!!!!!!!!!
> = 1 failed, 231 passed, 12 skipped, 1 xfailed, 154463 warnings in 107.50s (0:01:47) =The problem is that a fast enough machine winds up building python 3.12 and python 3.13 versions simultaneously. |
Oh, ok, then sure, let's skip this test. |
I'll try to look into that, when I get time. It may end up being out of scope for this PR.
I tried running nixpkgs-review on it, but ran into Mic92/nixpkgs-review#439 (logs here). I've not setup emulation yet and I don't have access to any remote builders, so I can't test ARM Linux locally.
I'll take another look when I get chance, but from a quick skim I can't see many inputs being removed in that diff. Thanks for steering me in the right direction. |
Looking at the logs in more detail, it looks like chromadb built fine, but vectorcode is failing to import it during its check phase. From the chromadb part of the trace:
try:
# Equivalent to import onnxruntime
self.ort = importlib.import_module("onnxruntime") # <-- here
except ImportError:
raise ValueError(
"The onnxruntime python package is not installed. Please install it with `pip install onnxruntime`"
)Could this be an ARM-specific bug in EDIT: No, we're running into microsoft/onnxruntime#10038 |
MattSturgeon
force-pushed
the
chromadb_0/tests
branch
from
61530e0 to
e2d9de5
Compare
|
Ah, I see. The base chromadb package has: nixpkgs/pkgs/development/python-modules/chromadb/default.nix Lines 172 to 174 in c42d484 However IIUC that actually means the package is broken on ARM linux, it is just hiding it by skipping the import check. If so, we should do: diff --git a/pkgs/development/python-modules/chromadb/default.nix b/pkgs/development/python-modules/chromadb/default.nix
index 868cd5a7ca42..704df33ccd31 100644
--- a/pkgs/development/python-modules/chromadb/default.nix
+++ b/pkgs/development/python-modules/chromadb/default.nix
@@ -169,9 +169,7 @@ buildPythonPackage rec {
starlette
];
- # Disable on aarch64-linux due to broken onnxruntime
- # https://github.com/microsoft/onnxruntime/issues/10038
- pythonImportsCheck = lib.optionals (stdenv.hostPlatform.system != "aarch64-linux") [ "chromadb" ];
+ pythonImportsCheck = [ "chromadb" ];
# Test collection breaks on aarch64-linux
doCheck = stdenv.hostPlatform.system != "aarch64-linux";
@@ -271,5 +269,8 @@ buildPythonPackage rec {
sarahec
];
mainProgram = "chroma";
+ # Broken on aarch64-linux due to onnxruntime issue:
+ # https://github.com/microsoft/onnxruntime/issues/10038
+ broken = with stdenv.hostPlatform; isAarch && isLinux;
};
}EDIT:It looks like the onnxruntime may also be masking issues, as it will usually have it's check phase disabled on aarch64-linux: nixpkgs/pkgs/by-name/on/onnxruntime/package.nix Lines 240 to 241 in 08f2208 I suspect it should also be marked broken on aarch64-linux, but I don't want this PR's scope to grow too much. cc onnxruntime maintainers: @puffnfresh @ck3d @cbourjau EDIT2:Some prior discussion on this in #412528 (comment) |
MattSturgeon
force-pushed
the
chromadb_0/tests
branch
from
e2d9de5 to
84474c8
Compare
|
|
Apologies for being short with people the last few days. Being sick apparently gives me a short temper. (Still recovering, but no longer coughing up a lung.) |
|
Interesting. So those packages must each either not invoke whichever part of chromadb imports onnxruntime during their tests, or disable any tests that do on aarch64-linux. |
|
re: python 3.12 and python 3.13 building concurrently: it's a known issue on Darwin, but I guess its not the issue here after all. I have a theory and I'll see if I can get |
As far as I can tell, this is happening only when 1) It was tempting to simply mark this as broken on aarch64-linux, but then I try to put myself in the user's shoes and go the extra mile to make it work (just as you did with tests ;) ). |
github-actions
bot
added
the
12.approvals: 1
|
On aarch64-linux onnxruntime cannot be imported, as it attempts to read /sys/devices/system/cpu, which does not exist in the sandbox. As onnxruntime cannot be imported, chromadb cannot be tested. However, this issue only applies in the build sandbox, not at runtime. Therefore the package doesn't need to be marked broken. Updated the condition to check `buildPlatform` instead of `hostPlatform`, as the issue relates to the build sandbox and is not a runtime issue. Use `isAarch` and `isLinux` attributes instead of doing string comparison. --- See NixOS#412528 (comment) And upstream onnxruntime issue: microsoft/onnxruntime#10038
It was accidentally in `nativeCheckInputs`, so the build fails when `doCheck = false`.
This allows accessing the overridden chromadb in the overridden python instance.
The tests need to import onnxruntime (a transitive of chromadb), however it has issues running in the build sandbox on aarch64-linux. It may work at runtime, so the package is not marked broken, however it will break tests, so the check phase is disabled when building on aarch64-linux. Since the issue comes via chromadb, we inherit its `doCheck` attribute.
Using a subset of disabled tests from the base package, as others only
apply to >1.0
---
Additionally, disable test_chroma.py and test_client.py:
These tests have issues running in parallel, such as when building the
python 3.12 and 3.13 versions simultaneously.
E.g. when running nixpkgs-review or unlucky timing in hydra.
Usually issues show up as:
ValueError: An instance of Chroma already exists for ephemeral with different settings
MattSturgeon
force-pushed
the
chromadb_0/tests
branch
from
a03032c to
3cf4c4b
Compare
|
Rebased to resolve conflicts |
|
@GaetanLepage please take a look |
nixpkgs-ci
bot
added
12.approvals: 2
|
Thanks @MattSturgeon for the very clean commit history with precise descriptions! |
|
4 participants
Follow up to #417720 (comment)
I've marked @sarahec as the author of 3cf4c4b, as it is lifted directly from their work in #412528. Note most of the changes in #421528 aren't needed because here we're overriding the v1.0 base package, and most things stay the same.
Additionally,
theThis was dropped in #412528, but the brokenness was just being hidden by disabling the import check on aarch64-linux. The package is still broken due to an onnxruntime issue (microsoft/onnxruntime#10038). EDIT: this issue only occurs in the build sandbox, so I haven't marked it as broken.brokenflag has been restored to chromadb on aarch64-linux.cc @fabaff @sarahec @dotlambda @GaetanLepage
Things done
vectorcode.chromadbandvectorcode.pythonpassthrus to expose overridden inputs and aid debuggingdoCheck = falseRestored chromadb'sbrokenflag on aarch64-linuxRemoved condition from chromadb'spythonImportsChecknix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.