array tests: handle different hexdigests from zlib-ng (#1678)

[AdamWill]

As explained in the issue, zlib-ng produces different hex digests from original zlib. This adjusts the tests slightly to allow for this.

TODO:

• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

Removed TODO items are irrelevant as this only changes tests. This is more or less the same as #1971 , but for the main (v2) branch rather than v3 branch.

[AdamWill]

If I run black on the changed file here locally it does indeed want to change a bunch of stuff, but none of it is stuff this PR touches - the issues already exist.

[d-v-b]

Thanks for the fix. For posterity, the ideal way to handle two different versions of zlib would be to condition the test case on the detected zlib version, but our test design makes that very tedious and not worth the effort. I think what you have done here is good!

[d-v-b]

we are seeing test failures due to numpy 2.0 I think

[AdamWill]

yeah, it looks like numpy got some custom types. I guess you might need to do stuff like:

diff --git a/src/zarr/v2/core.py b/src/zarr/v2/core.py
index c1223dac..04da6749 100644
--- a/src/zarr/v2/core.py
+++ b/src/zarr/v2/core.py
@@ -759,7 +759,7 @@ class Array:
Retrieve a single item::
- >>> z.get_basic_selection(5)
+ >>> int(z.get_basic_selection(5))
5
Retrieve a region via slicing::

or something along those lines.

[QuLogic]

I guess the NumPy stuff was fixed by #2073, so this might just need a rebase.

[AdamWill]

Rebased.

[dstansby]

Thanks for the PR - is there any way we can install zlib-ng and test against it in our continuous integration? I'm wary about adding these new digests without actually testing them to make sure they're correct.

[AdamWill]

Possibly by using this PPA. I can play around with it if I get time.

AFAIK github doesn't offer anything besides Ubuntu and Windows as hosted runners, so if you want to run on any other OS you have to self-host the runners, which is a whole thing.

[dstansby]

It looks like there's a package on PyPI: https://pypi.org/project/zlib-ng/, so perhaps we could install that on Ubuntu and test the new digests using that?

[AdamWill]

It's a bit confusing, but I think that's only python bindings. If you look at their CI, they install miniconda and then https://anaconda.org/conda-forge/zlib-ng to get zlib-ng itself before installing their own thing - https://github.com/pycompression/python-zlib-ng/blob/develop/.github/workflows/ci.yml#L120 . Presumably you'd also have to do that here. I kinda feel like using a PPA that replaces the system zlib seems more straightforward than doing that, but I haven't tried either way yet, tbf...

[AdamWill]

huh, so it looks like we already have miniconda in our CI anyway so it should be fairly trivial to add a matrix dimension that tests with zlib-ng from miniconda - that's what I tried to do here - but GHA doesn't seem to be generating that matrix combination. not sure if it needs admin approval or something?

[AdamWill]

this is a slightly different way which should avoid a bit of a combinatorial explosion effect, but GHA still doesn't seem to pick up the change :/

[jhamman]

I've moved the base branch of this PR to support/2.x in case there is interest in continuing this work.

[AdamWill]

well, I don't know why GHA isn't picking up the test matrix change, and I'm not an admin so I can't really poke about much and find out. I'm a bit stuck there.

[QuLogic]

I don't see any links to any workflow runs on the commit related to the changed file, so that suggests that perhaps there is a syntax error?

[AdamWill]

hum, well, I did see one thing (I had condadeps as an array in one case but a string in the other, it should be a string in both I think). not sure if that was the issue, though. edited and rebased.

[QuLogic]

This repo also has the stricter workflow approval setting enabled, so it's quite possible that that is interfering.

You may be able to confirm by pushing to main on your fork (and maybe confirming Actions settings in your fork).

[dstansby]

Hmm, I'm not even getting the option to approve the workflow runs. Let me close and re-open this PR to see if that helps.

[dstansby]

Ah, this is because we are in branch naming flux, and #2349 needs to get in before we can run actions on the v2 branch. Sorry about this, when it's working again I'll run the tests, and if they pass give this a merge. Thanks for the contribution and patience with us on this!

[jhamman]

CI should be up and running again now.

[AdamWill]

aaaagh merge commits, evil! i'll rebase.

[AdamWill]

the merge-commit version actually seems wrong as it's badly merged (it'll cause python 3.13 with numpy 1.24 to be included, not excluded).

[dstansby]

Sorry 🙈

[AdamWill]

Ugh. https://github.com/zarr-developers/zarr-python/actions/runs/11411354019 . What the heck? Why can't I use an empty string?

I was trying to avoid duplicating 'pip nodejs' in the two places we define the conda deps string, but if we can't use an empty string in one place I don't see how :(

[QuLogic]

It was right with the array, I think; as the matrix expands everything as the product of every list item. At least, all their examples use arrays, though none of them contain only a single item.

[AdamWill]

ok then, let's try them both as arrays...

[AdamWill]

ugh. no. I'm pretty sure it was right the first time: first def is an array, second is a string. back to that.

[dstansby]

Thanks again for this, and sorry it's taken so long - I will merge if CI passes.

[AdamWill]

As explained in the issue, zlib-ng produces different hex digests from original zlib. This adjusts the tests slightly to allow for this.

TODO:

• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

Removed TODO items are irrelevant as this only changes tests. This is more or less the same as #1971 , but for the main (v2) branch rather than v3 branch.

[AdamWill]

If I run black on the changed file here locally it does indeed want to change a bunch of stuff, but none of it is stuff this PR touches - the issues already exist.

[d-v-b]

Thanks for the fix. For posterity, the ideal way to handle two different versions of zlib would be to condition the test case on the detected zlib version, but our test design makes that very tedious and not worth the effort. I think what you have done here is good!

[d-v-b]

we are seeing test failures due to numpy 2.0 I think

[AdamWill]

yeah, it looks like numpy got some custom types. I guess you might need to do stuff like:

diff --git a/src/zarr/v2/core.py b/src/zarr/v2/core.py
index c1223dac..04da6749 100644
--- a/src/zarr/v2/core.py
+++ b/src/zarr/v2/core.py
@@ -759,7 +759,7 @@ class Array:
Retrieve a single item::
- >>> z.get_basic_selection(5)
+ >>> int(z.get_basic_selection(5))
5
Retrieve a region via slicing::

or something along those lines.

[QuLogic]

I guess the NumPy stuff was fixed by #2073, so this might just need a rebase.

[AdamWill]

Rebased.

[dstansby]

Thanks for the PR - is there any way we can install zlib-ng and test against it in our continuous integration? I'm wary about adding these new digests without actually testing them to make sure they're correct.

[AdamWill]

Possibly by using this PPA. I can play around with it if I get time.

AFAIK github doesn't offer anything besides Ubuntu and Windows as hosted runners, so if you want to run on any other OS you have to self-host the runners, which is a whole thing.

[dstansby]

It looks like there's a package on PyPI: https://pypi.org/project/zlib-ng/, so perhaps we could install that on Ubuntu and test the new digests using that?

[AdamWill]

It's a bit confusing, but I think that's only python bindings. If you look at their CI, they install miniconda and then https://anaconda.org/conda-forge/zlib-ng to get zlib-ng itself before installing their own thing - https://github.com/pycompression/python-zlib-ng/blob/develop/.github/workflows/ci.yml#L120 . Presumably you'd also have to do that here. I kinda feel like using a PPA that replaces the system zlib seems more straightforward than doing that, but I haven't tried either way yet, tbf...

[AdamWill]

huh, so it looks like we already have miniconda in our CI anyway so it should be fairly trivial to add a matrix dimension that tests with zlib-ng from miniconda - that's what I tried to do here - but GHA doesn't seem to be generating that matrix combination. not sure if it needs admin approval or something?

[AdamWill]

this is a slightly different way which should avoid a bit of a combinatorial explosion effect, but GHA still doesn't seem to pick up the change :/

[jhamman]

I've moved the base branch of this PR to support/2.x in case there is interest in continuing this work.

[AdamWill]

well, I don't know why GHA isn't picking up the test matrix change, and I'm not an admin so I can't really poke about much and find out. I'm a bit stuck there.

[QuLogic]

I don't see any links to any workflow runs on the commit related to the changed file, so that suggests that perhaps there is a syntax error?

[AdamWill]

hum, well, I did see one thing (I had condadeps as an array in one case but a string in the other, it should be a string in both I think). not sure if that was the issue, though. edited and rebased.

[QuLogic]

This repo also has the stricter workflow approval setting enabled, so it's quite possible that that is interfering.

You may be able to confirm by pushing to main on your fork (and maybe confirming Actions settings in your fork).

[dstansby]

Hmm, I'm not even getting the option to approve the workflow runs. Let me close and re-open this PR to see if that helps.

[dstansby]

Ah, this is because we are in branch naming flux, and #2349 needs to get in before we can run actions on the v2 branch. Sorry about this, when it's working again I'll run the tests, and if they pass give this a merge. Thanks for the contribution and patience with us on this!

[jhamman]

CI should be up and running again now.

[AdamWill]

aaaagh merge commits, evil! i'll rebase.

[AdamWill]

the merge-commit version actually seems wrong as it's badly merged (it'll cause python 3.13 with numpy 1.24 to be included, not excluded).

[dstansby]

Sorry 🙈

[AdamWill]

Ugh. https://github.com/zarr-developers/zarr-python/actions/runs/11411354019 . What the heck? Why can't I use an empty string?

I was trying to avoid duplicating 'pip nodejs' in the two places we define the conda deps string, but if we can't use an empty string in one place I don't see how :(

[QuLogic]

It was right with the array, I think; as the matrix expands everything as the product of every list item. At least, all their examples use arrays, though none of them contain only a single item.

[AdamWill]

ok then, let's try them both as arrays...

[AdamWill]

ugh. no. I'm pretty sure it was right the first time: first def is an array, second is a string. back to that.

[dstansby]

Thanks again for this, and sorry it's taken so long - I will merge if CI passes.

[AdamWill]

As explained in the issue, zlib-ng produces different hex digests from original zlib. This adjusts the tests slightly to allow for this.

TODO:

• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

Removed TODO items are irrelevant as this only changes tests. This is more or less the same as #1971 , but for the main (v2) branch rather than v3 branch.

[AdamWill]

If I run black on the changed file here locally it does indeed want to change a bunch of stuff, but none of it is stuff this PR touches - the issues already exist.

[d-v-b]

Thanks for the fix. For posterity, the ideal way to handle two different versions of zlib would be to condition the test case on the detected zlib version, but our test design makes that very tedious and not worth the effort. I think what you have done here is good!

[d-v-b]

we are seeing test failures due to numpy 2.0 I think

[AdamWill]

yeah, it looks like numpy got some custom types. I guess you might need to do stuff like:

diff --git a/src/zarr/v2/core.py b/src/zarr/v2/core.py
index c1223dac..04da6749 100644
--- a/src/zarr/v2/core.py
+++ b/src/zarr/v2/core.py
@@ -759,7 +759,7 @@ class Array:
Retrieve a single item::
- >>> z.get_basic_selection(5)
+ >>> int(z.get_basic_selection(5))
5
Retrieve a region via slicing::

or something along those lines.

[QuLogic]

I guess the NumPy stuff was fixed by #2073, so this might just need a rebase.

[AdamWill]

Rebased.

[dstansby]

Thanks for the PR - is there any way we can install zlib-ng and test against it in our continuous integration? I'm wary about adding these new digests without actually testing them to make sure they're correct.

[AdamWill]

Possibly by using this PPA. I can play around with it if I get time.

AFAIK github doesn't offer anything besides Ubuntu and Windows as hosted runners, so if you want to run on any other OS you have to self-host the runners, which is a whole thing.

[dstansby]

It looks like there's a package on PyPI: https://pypi.org/project/zlib-ng/, so perhaps we could install that on Ubuntu and test the new digests using that?

[AdamWill]

It's a bit confusing, but I think that's only python bindings. If you look at their CI, they install miniconda and then https://anaconda.org/conda-forge/zlib-ng to get zlib-ng itself before installing their own thing - https://github.com/pycompression/python-zlib-ng/blob/develop/.github/workflows/ci.yml#L120 . Presumably you'd also have to do that here. I kinda feel like using a PPA that replaces the system zlib seems more straightforward than doing that, but I haven't tried either way yet, tbf...

[AdamWill]

huh, so it looks like we already have miniconda in our CI anyway so it should be fairly trivial to add a matrix dimension that tests with zlib-ng from miniconda - that's what I tried to do here - but GHA doesn't seem to be generating that matrix combination. not sure if it needs admin approval or something?

[AdamWill]

this is a slightly different way which should avoid a bit of a combinatorial explosion effect, but GHA still doesn't seem to pick up the change :/

[jhamman]

I've moved the base branch of this PR to support/2.x in case there is interest in continuing this work.

[AdamWill]

well, I don't know why GHA isn't picking up the test matrix change, and I'm not an admin so I can't really poke about much and find out. I'm a bit stuck there.

[QuLogic]

I don't see any links to any workflow runs on the commit related to the changed file, so that suggests that perhaps there is a syntax error?

[AdamWill]

hum, well, I did see one thing (I had condadeps as an array in one case but a string in the other, it should be a string in both I think). not sure if that was the issue, though. edited and rebased.

[QuLogic]

This repo also has the stricter workflow approval setting enabled, so it's quite possible that that is interfering.

You may be able to confirm by pushing to main on your fork (and maybe confirming Actions settings in your fork).

[dstansby]

Hmm, I'm not even getting the option to approve the workflow runs. Let me close and re-open this PR to see if that helps.

[dstansby]

Ah, this is because we are in branch naming flux, and #2349 needs to get in before we can run actions on the v2 branch. Sorry about this, when it's working again I'll run the tests, and if they pass give this a merge. Thanks for the contribution and patience with us on this!

[jhamman]

CI should be up and running again now.

[AdamWill]

aaaagh merge commits, evil! i'll rebase.

[AdamWill]

the merge-commit version actually seems wrong as it's badly merged (it'll cause python 3.13 with numpy 1.24 to be included, not excluded).

[dstansby]

Sorry 🙈

[AdamWill]

Ugh. https://github.com/zarr-developers/zarr-python/actions/runs/11411354019 . What the heck? Why can't I use an empty string?

I was trying to avoid duplicating 'pip nodejs' in the two places we define the conda deps string, but if we can't use an empty string in one place I don't see how :(

[QuLogic]

It was right with the array, I think; as the matrix expands everything as the product of every list item. At least, all their examples use arrays, though none of them contain only a single item.

[AdamWill]

ok then, let's try them both as arrays...

[AdamWill]

ugh. no. I'm pretty sure it was right the first time: first def is an array, second is a string. back to that.

[dstansby]

Thanks again for this, and sorry it's taken so long - I will merge if CI passes.

[AdamWill]

As explained in the issue, zlib-ng produces different hex digests from original zlib. This adjusts the tests slightly to allow for this.

TODO:

• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

Removed TODO items are irrelevant as this only changes tests. This is more or less the same as #1971 , but for the main (v2) branch rather than v3 branch.

[AdamWill]

If I run black on the changed file here locally it does indeed want to change a bunch of stuff, but none of it is stuff this PR touches - the issues already exist.

[d-v-b]

Thanks for the fix. For posterity, the ideal way to handle two different versions of zlib would be to condition the test case on the detected zlib version, but our test design makes that very tedious and not worth the effort. I think what you have done here is good!

[d-v-b]

we are seeing test failures due to numpy 2.0 I think

[AdamWill]

yeah, it looks like numpy got some custom types. I guess you might need to do stuff like:

diff --git a/src/zarr/v2/core.py b/src/zarr/v2/core.py
index c1223dac..04da6749 100644
--- a/src/zarr/v2/core.py
+++ b/src/zarr/v2/core.py
@@ -759,7 +759,7 @@ class Array:
Retrieve a single item::
- >>> z.get_basic_selection(5)
+ >>> int(z.get_basic_selection(5))
5
Retrieve a region via slicing::

or something along those lines.

[QuLogic]

I guess the NumPy stuff was fixed by #2073, so this might just need a rebase.

[AdamWill]

Rebased.

[dstansby]

Thanks for the PR - is there any way we can install zlib-ng and test against it in our continuous integration? I'm wary about adding these new digests without actually testing them to make sure they're correct.

[AdamWill]

Possibly by using this PPA. I can play around with it if I get time.

AFAIK github doesn't offer anything besides Ubuntu and Windows as hosted runners, so if you want to run on any other OS you have to self-host the runners, which is a whole thing.

[dstansby]

It looks like there's a package on PyPI: https://pypi.org/project/zlib-ng/, so perhaps we could install that on Ubuntu and test the new digests using that?

[AdamWill]

It's a bit confusing, but I think that's only python bindings. If you look at their CI, they install miniconda and then https://anaconda.org/conda-forge/zlib-ng to get zlib-ng itself before installing their own thing - https://github.com/pycompression/python-zlib-ng/blob/develop/.github/workflows/ci.yml#L120 . Presumably you'd also have to do that here. I kinda feel like using a PPA that replaces the system zlib seems more straightforward than doing that, but I haven't tried either way yet, tbf...

[AdamWill]

huh, so it looks like we already have miniconda in our CI anyway so it should be fairly trivial to add a matrix dimension that tests with zlib-ng from miniconda - that's what I tried to do here - but GHA doesn't seem to be generating that matrix combination. not sure if it needs admin approval or something?

[AdamWill]

this is a slightly different way which should avoid a bit of a combinatorial explosion effect, but GHA still doesn't seem to pick up the change :/

[jhamman]

I've moved the base branch of this PR to support/2.x in case there is interest in continuing this work.

[AdamWill]

well, I don't know why GHA isn't picking up the test matrix change, and I'm not an admin so I can't really poke about much and find out. I'm a bit stuck there.

[QuLogic]

I don't see any links to any workflow runs on the commit related to the changed file, so that suggests that perhaps there is a syntax error?

[AdamWill]

hum, well, I did see one thing (I had condadeps as an array in one case but a string in the other, it should be a string in both I think). not sure if that was the issue, though. edited and rebased.

[QuLogic]

This repo also has the stricter workflow approval setting enabled, so it's quite possible that that is interfering.

You may be able to confirm by pushing to main on your fork (and maybe confirming Actions settings in your fork).

[dstansby]

Hmm, I'm not even getting the option to approve the workflow runs. Let me close and re-open this PR to see if that helps.

[dstansby]

Ah, this is because we are in branch naming flux, and #2349 needs to get in before we can run actions on the v2 branch. Sorry about this, when it's working again I'll run the tests, and if they pass give this a merge. Thanks for the contribution and patience with us on this!

[jhamman]

CI should be up and running again now.

[AdamWill]

aaaagh merge commits, evil! i'll rebase.

[AdamWill]

the merge-commit version actually seems wrong as it's badly merged (it'll cause python 3.13 with numpy 1.24 to be included, not excluded).

[dstansby]

Sorry 🙈

[AdamWill]

Ugh. https://github.com/zarr-developers/zarr-python/actions/runs/11411354019 . What the heck? Why can't I use an empty string?

I was trying to avoid duplicating 'pip nodejs' in the two places we define the conda deps string, but if we can't use an empty string in one place I don't see how :(

[QuLogic]

It was right with the array, I think; as the matrix expands everything as the product of every list item. At least, all their examples use arrays, though none of them contain only a single item.

[AdamWill]

ok then, let's try them both as arrays...

[AdamWill]

ugh. no. I'm pretty sure it was right the first time: first def is an array, second is a string. back to that.

[dstansby]

Thanks again for this, and sorry it's taken so long - I will merge if CI passes.

[AdamWill]

As explained in the issue, zlib-ng produces different hex digests from original zlib. This adjusts the tests slightly to allow for this.

TODO:

• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

Removed TODO items are irrelevant as this only changes tests. This is more or less the same as #1971 , but for the main (v2) branch rather than v3 branch.

[AdamWill]

If I run black on the changed file here locally it does indeed want to change a bunch of stuff, but none of it is stuff this PR touches - the issues already exist.

[d-v-b]

Thanks for the fix. For posterity, the ideal way to handle two different versions of zlib would be to condition the test case on the detected zlib version, but our test design makes that very tedious and not worth the effort. I think what you have done here is good!

[d-v-b]

we are seeing test failures due to numpy 2.0 I think

[AdamWill]

yeah, it looks like numpy got some custom types. I guess you might need to do stuff like:

diff --git a/src/zarr/v2/core.py b/src/zarr/v2/core.py
index c1223dac..04da6749 100644
--- a/src/zarr/v2/core.py
+++ b/src/zarr/v2/core.py
@@ -759,7 +759,7 @@ class Array:
Retrieve a single item::
- >>> z.get_basic_selection(5)
+ >>> int(z.get_basic_selection(5))
5
Retrieve a region via slicing::

or something along those lines.

[QuLogic]

I guess the NumPy stuff was fixed by #2073, so this might just need a rebase.

[AdamWill]

Rebased.

[dstansby]

Thanks for the PR - is there any way we can install zlib-ng and test against it in our continuous integration? I'm wary about adding these new digests without actually testing them to make sure they're correct.

[AdamWill]

Possibly by using this PPA. I can play around with it if I get time.

AFAIK github doesn't offer anything besides Ubuntu and Windows as hosted runners, so if you want to run on any other OS you have to self-host the runners, which is a whole thing.

[dstansby]

It looks like there's a package on PyPI: https://pypi.org/project/zlib-ng/, so perhaps we could install that on Ubuntu and test the new digests using that?

[AdamWill]

It's a bit confusing, but I think that's only python bindings. If you look at their CI, they install miniconda and then https://anaconda.org/conda-forge/zlib-ng to get zlib-ng itself before installing their own thing - https://github.com/pycompression/python-zlib-ng/blob/develop/.github/workflows/ci.yml#L120 . Presumably you'd also have to do that here. I kinda feel like using a PPA that replaces the system zlib seems more straightforward than doing that, but I haven't tried either way yet, tbf...

[AdamWill]

huh, so it looks like we already have miniconda in our CI anyway so it should be fairly trivial to add a matrix dimension that tests with zlib-ng from miniconda - that's what I tried to do here - but GHA doesn't seem to be generating that matrix combination. not sure if it needs admin approval or something?

[AdamWill]

this is a slightly different way which should avoid a bit of a combinatorial explosion effect, but GHA still doesn't seem to pick up the change :/

[jhamman]

I've moved the base branch of this PR to support/2.x in case there is interest in continuing this work.

[AdamWill]

well, I don't know why GHA isn't picking up the test matrix change, and I'm not an admin so I can't really poke about much and find out. I'm a bit stuck there.

[QuLogic]

I don't see any links to any workflow runs on the commit related to the changed file, so that suggests that perhaps there is a syntax error?

[AdamWill]

hum, well, I did see one thing (I had condadeps as an array in one case but a string in the other, it should be a string in both I think). not sure if that was the issue, though. edited and rebased.

[QuLogic]

This repo also has the stricter workflow approval setting enabled, so it's quite possible that that is interfering.

You may be able to confirm by pushing to main on your fork (and maybe confirming Actions settings in your fork).

[dstansby]

Hmm, I'm not even getting the option to approve the workflow runs. Let me close and re-open this PR to see if that helps.

[dstansby]

Ah, this is because we are in branch naming flux, and #2349 needs to get in before we can run actions on the v2 branch. Sorry about this, when it's working again I'll run the tests, and if they pass give this a merge. Thanks for the contribution and patience with us on this!

[jhamman]

CI should be up and running again now.

[AdamWill]

aaaagh merge commits, evil! i'll rebase.

[AdamWill]

the merge-commit version actually seems wrong as it's badly merged (it'll cause python 3.13 with numpy 1.24 to be included, not excluded).

[dstansby]

Sorry 🙈

[AdamWill]

Ugh. https://github.com/zarr-developers/zarr-python/actions/runs/11411354019 . What the heck? Why can't I use an empty string?

I was trying to avoid duplicating 'pip nodejs' in the two places we define the conda deps string, but if we can't use an empty string in one place I don't see how :(

[QuLogic]

It was right with the array, I think; as the matrix expands everything as the product of every list item. At least, all their examples use arrays, though none of them contain only a single item.

[AdamWill]

ok then, let's try them both as arrays...

[AdamWill]

ugh. no. I'm pretty sure it was right the first time: first def is an array, second is a string. back to that.

[dstansby]

Thanks again for this, and sorry it's taken so long - I will merge if CI passes.

[AdamWill]

As explained in the issue, zlib-ng produces different hex digests from original zlib. This adjusts the tests slightly to allow for this.

TODO:

• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

Removed TODO items are irrelevant as this only changes tests. This is more or less the same as #1971 , but for the main (v2) branch rather than v3 branch.

[AdamWill]

If I run black on the changed file here locally it does indeed want to change a bunch of stuff, but none of it is stuff this PR touches - the issues already exist.

[d-v-b]

Thanks for the fix. For posterity, the ideal way to handle two different versions of zlib would be to condition the test case on the detected zlib version, but our test design makes that very tedious and not worth the effort. I think what you have done here is good!

[d-v-b]

we are seeing test failures due to numpy 2.0 I think

[AdamWill]

yeah, it looks like numpy got some custom types. I guess you might need to do stuff like:

diff --git a/src/zarr/v2/core.py b/src/zarr/v2/core.py
index c1223dac..04da6749 100644
--- a/src/zarr/v2/core.py
+++ b/src/zarr/v2/core.py
@@ -759,7 +759,7 @@ class Array:
Retrieve a single item::
- >>> z.get_basic_selection(5)
+ >>> int(z.get_basic_selection(5))
5
Retrieve a region via slicing::

or something along those lines.

[QuLogic]

I guess the NumPy stuff was fixed by #2073, so this might just need a rebase.

[AdamWill]

Rebased.

[dstansby]

Thanks for the PR - is there any way we can install zlib-ng and test against it in our continuous integration? I'm wary about adding these new digests without actually testing them to make sure they're correct.

[AdamWill]

Possibly by using this PPA. I can play around with it if I get time.

AFAIK github doesn't offer anything besides Ubuntu and Windows as hosted runners, so if you want to run on any other OS you have to self-host the runners, which is a whole thing.

[dstansby]

It looks like there's a package on PyPI: https://pypi.org/project/zlib-ng/, so perhaps we could install that on Ubuntu and test the new digests using that?

[AdamWill]

It's a bit confusing, but I think that's only python bindings. If you look at their CI, they install miniconda and then https://anaconda.org/conda-forge/zlib-ng to get zlib-ng itself before installing their own thing - https://github.com/pycompression/python-zlib-ng/blob/develop/.github/workflows/ci.yml#L120 . Presumably you'd also have to do that here. I kinda feel like using a PPA that replaces the system zlib seems more straightforward than doing that, but I haven't tried either way yet, tbf...

[AdamWill]

huh, so it looks like we already have miniconda in our CI anyway so it should be fairly trivial to add a matrix dimension that tests with zlib-ng from miniconda - that's what I tried to do here - but GHA doesn't seem to be generating that matrix combination. not sure if it needs admin approval or something?

[AdamWill]

this is a slightly different way which should avoid a bit of a combinatorial explosion effect, but GHA still doesn't seem to pick up the change :/

[jhamman]

I've moved the base branch of this PR to support/2.x in case there is interest in continuing this work.

[AdamWill]

well, I don't know why GHA isn't picking up the test matrix change, and I'm not an admin so I can't really poke about much and find out. I'm a bit stuck there.

[QuLogic]

I don't see any links to any workflow runs on the commit related to the changed file, so that suggests that perhaps there is a syntax error?

[AdamWill]

hum, well, I did see one thing (I had condadeps as an array in one case but a string in the other, it should be a string in both I think). not sure if that was the issue, though. edited and rebased.

[QuLogic]

This repo also has the stricter workflow approval setting enabled, so it's quite possible that that is interfering.

You may be able to confirm by pushing to main on your fork (and maybe confirming Actions settings in your fork).

[dstansby]

Hmm, I'm not even getting the option to approve the workflow runs. Let me close and re-open this PR to see if that helps.

[dstansby]

Ah, this is because we are in branch naming flux, and #2349 needs to get in before we can run actions on the v2 branch. Sorry about this, when it's working again I'll run the tests, and if they pass give this a merge. Thanks for the contribution and patience with us on this!

[jhamman]

CI should be up and running again now.

[AdamWill]

aaaagh merge commits, evil! i'll rebase.

[AdamWill]

the merge-commit version actually seems wrong as it's badly merged (it'll cause python 3.13 with numpy 1.24 to be included, not excluded).

[dstansby]

Sorry 🙈

[AdamWill]

Ugh. https://github.com/zarr-developers/zarr-python/actions/runs/11411354019 . What the heck? Why can't I use an empty string?

I was trying to avoid duplicating 'pip nodejs' in the two places we define the conda deps string, but if we can't use an empty string in one place I don't see how :(

[QuLogic]

It was right with the array, I think; as the matrix expands everything as the product of every list item. At least, all their examples use arrays, though none of them contain only a single item.

[AdamWill]

ok then, let's try them both as arrays...

[AdamWill]

ugh. no. I'm pretty sure it was right the first time: first def is an array, second is a string. back to that.

[dstansby]

Thanks again for this, and sorry it's taken so long - I will merge if CI passes.

[AdamWill]

As explained in the issue, zlib-ng produces different hex digests from original zlib. This adjusts the tests slightly to allow for this.

TODO:

• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

Removed TODO items are irrelevant as this only changes tests. This is more or less the same as #1971 , but for the main (v2) branch rather than v3 branch.

[AdamWill]

If I run black on the changed file here locally it does indeed want to change a bunch of stuff, but none of it is stuff this PR touches - the issues already exist.

[d-v-b]

Thanks for the fix. For posterity, the ideal way to handle two different versions of zlib would be to condition the test case on the detected zlib version, but our test design makes that very tedious and not worth the effort. I think what you have done here is good!

[d-v-b]

we are seeing test failures due to numpy 2.0 I think

[AdamWill]

yeah, it looks like numpy got some custom types. I guess you might need to do stuff like:

diff --git a/src/zarr/v2/core.py b/src/zarr/v2/core.py
index c1223dac..04da6749 100644
--- a/src/zarr/v2/core.py
+++ b/src/zarr/v2/core.py
@@ -759,7 +759,7 @@ class Array:
Retrieve a single item::
- >>> z.get_basic_selection(5)
+ >>> int(z.get_basic_selection(5))
5
Retrieve a region via slicing::

or something along those lines.

[QuLogic]

I guess the NumPy stuff was fixed by #2073, so this might just need a rebase.

[AdamWill]

Rebased.

[dstansby]

Thanks for the PR - is there any way we can install zlib-ng and test against it in our continuous integration? I'm wary about adding these new digests without actually testing them to make sure they're correct.

[AdamWill]

Possibly by using this PPA. I can play around with it if I get time.

AFAIK github doesn't offer anything besides Ubuntu and Windows as hosted runners, so if you want to run on any other OS you have to self-host the runners, which is a whole thing.

[dstansby]

It looks like there's a package on PyPI: https://pypi.org/project/zlib-ng/, so perhaps we could install that on Ubuntu and test the new digests using that?

[AdamWill]

It's a bit confusing, but I think that's only python bindings. If you look at their CI, they install miniconda and then https://anaconda.org/conda-forge/zlib-ng to get zlib-ng itself before installing their own thing - https://github.com/pycompression/python-zlib-ng/blob/develop/.github/workflows/ci.yml#L120 . Presumably you'd also have to do that here. I kinda feel like using a PPA that replaces the system zlib seems more straightforward than doing that, but I haven't tried either way yet, tbf...

[AdamWill]

huh, so it looks like we already have miniconda in our CI anyway so it should be fairly trivial to add a matrix dimension that tests with zlib-ng from miniconda - that's what I tried to do here - but GHA doesn't seem to be generating that matrix combination. not sure if it needs admin approval or something?

[AdamWill]

this is a slightly different way which should avoid a bit of a combinatorial explosion effect, but GHA still doesn't seem to pick up the change :/

[jhamman]

I've moved the base branch of this PR to support/2.x in case there is interest in continuing this work.

[AdamWill]

well, I don't know why GHA isn't picking up the test matrix change, and I'm not an admin so I can't really poke about much and find out. I'm a bit stuck there.

[QuLogic]

I don't see any links to any workflow runs on the commit related to the changed file, so that suggests that perhaps there is a syntax error?

[AdamWill]

hum, well, I did see one thing (I had condadeps as an array in one case but a string in the other, it should be a string in both I think). not sure if that was the issue, though. edited and rebased.

[QuLogic]

This repo also has the stricter workflow approval setting enabled, so it's quite possible that that is interfering.

You may be able to confirm by pushing to main on your fork (and maybe confirming Actions settings in your fork).

[dstansby]

Hmm, I'm not even getting the option to approve the workflow runs. Let me close and re-open this PR to see if that helps.

[dstansby]

Ah, this is because we are in branch naming flux, and #2349 needs to get in before we can run actions on the v2 branch. Sorry about this, when it's working again I'll run the tests, and if they pass give this a merge. Thanks for the contribution and patience with us on this!

[jhamman]

CI should be up and running again now.

[AdamWill]

aaaagh merge commits, evil! i'll rebase.

[AdamWill]

the merge-commit version actually seems wrong as it's badly merged (it'll cause python 3.13 with numpy 1.24 to be included, not excluded).

[dstansby]

Sorry 🙈

[AdamWill]

Ugh. https://github.com/zarr-developers/zarr-python/actions/runs/11411354019 . What the heck? Why can't I use an empty string?

I was trying to avoid duplicating 'pip nodejs' in the two places we define the conda deps string, but if we can't use an empty string in one place I don't see how :(

[QuLogic]

It was right with the array, I think; as the matrix expands everything as the product of every list item. At least, all their examples use arrays, though none of them contain only a single item.

[AdamWill]

ok then, let's try them both as arrays...

[AdamWill]

ugh. no. I'm pretty sure it was right the first time: first def is an array, second is a string. back to that.

[dstansby]

Thanks again for this, and sorry it's taken so long - I will merge if CI passes.

[AdamWill]

As explained in the issue, zlib-ng produces different hex digests from original zlib. This adjusts the tests slightly to allow for this.

TODO:

• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

Removed TODO items are irrelevant as this only changes tests. This is more or less the same as #1971 , but for the main (v2) branch rather than v3 branch.

[AdamWill]

If I run black on the changed file here locally it does indeed want to change a bunch of stuff, but none of it is stuff this PR touches - the issues already exist.

[d-v-b]

Thanks for the fix. For posterity, the ideal way to handle two different versions of zlib would be to condition the test case on the detected zlib version, but our test design makes that very tedious and not worth the effort. I think what you have done here is good!

[d-v-b]

we are seeing test failures due to numpy 2.0 I think

[AdamWill]

yeah, it looks like numpy got some custom types. I guess you might need to do stuff like:

diff --git a/src/zarr/v2/core.py b/src/zarr/v2/core.py
index c1223dac..04da6749 100644
--- a/src/zarr/v2/core.py
+++ b/src/zarr/v2/core.py
@@ -759,7 +759,7 @@ class Array:
Retrieve a single item::
- >>> z.get_basic_selection(5)
+ >>> int(z.get_basic_selection(5))
5
Retrieve a region via slicing::

or something along those lines.

[QuLogic]

I guess the NumPy stuff was fixed by #2073, so this might just need a rebase.

[AdamWill]

Rebased.

[dstansby]

Thanks for the PR - is there any way we can install zlib-ng and test against it in our continuous integration? I'm wary about adding these new digests without actually testing them to make sure they're correct.

[AdamWill]

Possibly by using this PPA. I can play around with it if I get time.

AFAIK github doesn't offer anything besides Ubuntu and Windows as hosted runners, so if you want to run on any other OS you have to self-host the runners, which is a whole thing.

[dstansby]

It looks like there's a package on PyPI: https://pypi.org/project/zlib-ng/, so perhaps we could install that on Ubuntu and test the new digests using that?

[AdamWill]

It's a bit confusing, but I think that's only python bindings. If you look at their CI, they install miniconda and then https://anaconda.org/conda-forge/zlib-ng to get zlib-ng itself before installing their own thing - https://github.com/pycompression/python-zlib-ng/blob/develop/.github/workflows/ci.yml#L120 . Presumably you'd also have to do that here. I kinda feel like using a PPA that replaces the system zlib seems more straightforward than doing that, but I haven't tried either way yet, tbf...

[AdamWill]

huh, so it looks like we already have miniconda in our CI anyway so it should be fairly trivial to add a matrix dimension that tests with zlib-ng from miniconda - that's what I tried to do here - but GHA doesn't seem to be generating that matrix combination. not sure if it needs admin approval or something?

[AdamWill]

this is a slightly different way which should avoid a bit of a combinatorial explosion effect, but GHA still doesn't seem to pick up the change :/

[jhamman]

I've moved the base branch of this PR to support/2.x in case there is interest in continuing this work.

[AdamWill]

well, I don't know why GHA isn't picking up the test matrix change, and I'm not an admin so I can't really poke about much and find out. I'm a bit stuck there.

[QuLogic]

I don't see any links to any workflow runs on the commit related to the changed file, so that suggests that perhaps there is a syntax error?

[AdamWill]

hum, well, I did see one thing (I had condadeps as an array in one case but a string in the other, it should be a string in both I think). not sure if that was the issue, though. edited and rebased.

[QuLogic]

This repo also has the stricter workflow approval setting enabled, so it's quite possible that that is interfering.

You may be able to confirm by pushing to main on your fork (and maybe confirming Actions settings in your fork).

[dstansby]

Hmm, I'm not even getting the option to approve the workflow runs. Let me close and re-open this PR to see if that helps.

[dstansby]

Ah, this is because we are in branch naming flux, and #2349 needs to get in before we can run actions on the v2 branch. Sorry about this, when it's working again I'll run the tests, and if they pass give this a merge. Thanks for the contribution and patience with us on this!

[jhamman]

CI should be up and running again now.

[AdamWill]

aaaagh merge commits, evil! i'll rebase.

[AdamWill]

the merge-commit version actually seems wrong as it's badly merged (it'll cause python 3.13 with numpy 1.24 to be included, not excluded).

[dstansby]

Sorry 🙈

[AdamWill]

Ugh. https://github.com/zarr-developers/zarr-python/actions/runs/11411354019 . What the heck? Why can't I use an empty string?

I was trying to avoid duplicating 'pip nodejs' in the two places we define the conda deps string, but if we can't use an empty string in one place I don't see how :(

[QuLogic]

It was right with the array, I think; as the matrix expands everything as the product of every list item. At least, all their examples use arrays, though none of them contain only a single item.

[AdamWill]

ok then, let's try them both as arrays...

[AdamWill]

ugh. no. I'm pretty sure it was right the first time: first def is an array, second is a string. back to that.

[dstansby]

Thanks again for this, and sorry it's taken so long - I will merge if CI passes.