Handle legacy nested datasets (with tests) - #850

Merged
joshmoore merged 7 commits into
zarr-developers:masterfrom
joshmoore:issue-840
Oct 19, 2021
Merged

Handle legacy nested datasets (with tests)#850
joshmoore merged 7 commits into
zarr-developers:masterfrom
joshmoore:issue-840

Conversation

@joshmoore

@joshmoorejoshmoore commented Oct 18, 2021•
edited
Loading

Copy link
Copy Markdown
Member

fix#840
As pointed out @abred, zarr 2.10 (and earlier) was no longer properly handling the opening of nested datasets if they did not contain the dimension_separator metadata in .zarray. This is fixed with this PR. However, there are a number of cases which still xfail:

$ pytest zarr/tests/test_dim_separator.py -sv | grep XFAIL
zarr/tests/test_dim_separator.py::test_open[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_fsstore[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_directory[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[static_flat_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[directory_default] XFAIL
zarr/tests/test_dim_separator.py::test_nested[fs_default] XFAIL

Essentially for legacy datasets without the dimension_separator the following holds:

  • All nested datasets MUST be opened with NestedDirectoryStore.
  • All flat datasets MUST NOT be opened with NestedDirectoryStore.

These restrictions can be circumvented by updating your .zarray files to contain the proper metadata:

$ git grep dimension_separator fixture/{nested,flat}{,_legacy}
fixture/flat/.zarray: "dimension_separator": ".",
fixture/nested/.zarray: "dimension_separator": "/",

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@joshmoore
Fixzarr-developers#840
c36ec3d
@joshmoore
Add legacy tests (Seezarr-developers#840)
a35a1a5
Each fixture (flat & nested) should have a version
which does not include any new metadata.
@pep8speaks

pep8speaks commented Oct 18, 2021•
edited
Loading

Copy link
Copy Markdown

Hello @joshmoore! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-10-19 19:48:47 UTC

@joshmoorejoshmoore mentioned this pull request Oct 18, 2021
Closed
@codecov

codecovBot commented Oct 19, 2021•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #850 (6754503) into master (4f8cb35) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 6754503 differs from pull request most recent head ff1b9a9. Consider uploading reports for the commit ff1b9a9 to get more accurate results

@@ Coverage Diff @@## master #850 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10936 10952 +16 =======================================
+ Hits 10930 10946 +16 
Misses 6 6
Impacted FilesCoverage Δ
zarr/core.py100.00% <100.00%> (ø)
zarr/meta.py100.00% <100.00%> (ø)
zarr/tests/test_dim_separator.py100.00% <100.00%> (ø)

@joshmoore

joshmoore commented Oct 19, 2021•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

I'd propose to get this out as a quick 2.10.2 if there are no objections.

@joshmoore
joshmoore merged commit d1dc987 into zarr-developers:masterOct 19, 2021
@joshmoore
joshmoore deleted the issue-840 branch October 19, 2021 19:49
@jakirkham

jakirkham commented Oct 19, 2021

Copy link
Copy Markdown
Member

Thanks Josh! 😄

@joshmoore

joshmoore commented Oct 20, 2021

Copy link
Copy Markdown
MemberAuthor

Note that one of the tests that passed here is failing in the feedstock:

1 similar comment
@joshmoore

joshmoore commented Oct 20, 2021

Copy link
Copy Markdown
MemberAuthor

Note that one of the tests that passed here is failing in the feedstock:

jakirkham
jakirkham reviewed Oct 21, 2021
View reviewed changes
Comment thread.github/workflows/minimal.yml
run: |
conda activate minimal
python -m pip install -e .
python -m pip install .

@jakirkhamjakirkhamOct 21, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, why this was needed? Did it fail without this? If so, that may be indicative of the issue we are seeing in conda-forge

@joshmoorejoshmooreOct 21, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping the editable install fixed the error "No module named 'pip'". More info in pypa/pip#10573

@jakirkhamjakirkham mentioned this pull request Oct 21, 2021
Closed
3 tasks
@joshmoorejoshmoore mentioned this pull request Feb 9, 2022
Open
19 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jakirkhamjakirkhamjakirkham left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

zarr 2.10.0 fails to load pre-existing nested files

3 participants

@joshmoore@pep8speaks@jakirkham
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Handle legacy nested datasets (with tests) - #850
Merged
joshmoore merged 7 commits into
zarr-developers:masterfrom 
joshmoore:issue-840
Oct 19, 2021
Merged
Handle legacy nested datasets (with tests)#850
joshmoore merged 7 commits into
zarr-developers:masterfrom 
joshmoore:issue-840
Conversation
@joshmoore
@joshmoorejoshmoore
commented
Oct 18, 2021•

edited

Loading

Copy link


Copy Markdown

Member
fix#840

As pointed out @abred, zarr 2.10 (and earlier) was no longer properly handling the opening of nested datasets if they did not contain the dimension_separator metadata in .zarray. This is fixed with this PR. However, there are a number of cases which still xfail:
$ pytest zarr/tests/test_dim_separator.py -sv | grep XFAIL
zarr/tests/test_dim_separator.py::test_open[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_fsstore[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_directory[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[static_flat_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[directory_default] XFAIL
zarr/tests/test_dim_separator.py::test_nested[fs_default] XFAIL

Essentially for legacy datasets without the dimension_separator the following holds:
  • All nested datasets MUST be opened with NestedDirectoryStore.
  • All flat datasets MUST NOT be opened with NestedDirectoryStore.
These restrictions can be circumvented by updating your .zarray files to contain the proper metadata:
$ git grep dimension_separator fixture/{nested,flat}{,_legacy}
fixture/flat/.zarray: "dimension_separator": ".",
fixture/nested/.zarray: "dimension_separator": "/",

TODO:
  •  Add unit tests and/or doctests in docstrings
  •  Add docstrings and API docs for any new/modified user-facing classes and functions
  •  New/modified features documented in docs/tutorial.rst
  •  Changes documented in docs/release.rst
  •  GitHub Actions have all passed
  •  Test coverage is 100% (Codecov passes)
@joshmoore
Fixzarr-developers#840
c36ec3d
@joshmoore
Add legacy tests (Seezarr-developers#840)
a35a1a5
Each fixture (flat & nested) should have a version
which does not include any new metadata.
@pep8speaks
pep8speaks
commented
Oct 18, 2021•

edited

Loading

Copy link


Copy Markdown

Hello @joshmoore! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻
Comment last updated at 2021-10-19 19:48:47 UTC
@joshmoorejoshmoore
mentioned this pull request
Oct 18, 2021

Closed

@codecov
codecovBot
commented
Oct 19, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #850 (6754503) into master (4f8cb35) will increase coverage by 0.00%.

The diff coverage is 100.00%.
❗ Current head 6754503 differs from pull request most recent head ff1b9a9. Consider uploading reports for the commit ff1b9a9 to get more accurate results
@@ Coverage Diff @@## master #850 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10936 10952 +16 =======================================
+ Hits 10930 10946 +16 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/core.py100.00% <100.00%> (ø)
zarr/meta.py100.00% <100.00%> (ø)
zarr/tests/test_dim_separator.py100.00% <100.00%> (ø)
@joshmoore
joshmoore
commented
Oct 19, 2021•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'd propose to get this out as a quick 2.10.2 if there are no objections.
@joshmoore
joshmoore
merged commit d1dc987
into
zarr-developers:masterOct 19, 2021
@joshmoore
joshmoore
deleted the

issue-840

branch
October 19, 2021 19:49
@jakirkham
jakirkham
commented
Oct 19, 2021

Copy link


Copy Markdown

Member
Thanks Josh! 😄
@joshmoore
joshmoore
commented
Oct 20, 2021

Copy link


Copy Markdown

MemberAuthor
Note that one of the tests that passed here is failing in the feedstock:

1 similar comment

@joshmoore
joshmoore
commented
Oct 20, 2021

Copy link


Copy Markdown

MemberAuthor
Note that one of the tests that passed here is failing in the feedstock:
jakirkham
jakirkham
reviewed
Oct 21, 2021
View reviewed changes
Comment thread.github/workflows/minimal.yml
 run: |
 conda activate minimal
 python -m pip install -e .
 python -m pip install .
@jakirkhamjakirkhamOct 21, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, why this was needed? Did it fail without this? If so, that may be indicative of the issue we are seeing in conda-forge
@joshmoorejoshmooreOct 21, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping the editable install fixed the error "No module named 'pip'". More info in pypa/pip#10573
@jakirkhamjakirkham
mentioned this pull request
Oct 21, 2021
 Closed

3 tasks
@joshmoorejoshmoore
mentioned this pull request
Feb 9, 2022

Open

19 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jakirkhamjakirkhamjakirkham left review comments

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

zarr 2.10.0 fails to load pre-existing nested files


3 participants

@joshmoore@pep8speaks@jakirkham

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Handle legacy nested datasets (with tests) - #850
Merged
joshmoore merged 7 commits into
zarr-developers:masterfrom 
joshmoore:issue-840
Oct 19, 2021
Merged
Handle legacy nested datasets (with tests)#850
joshmoore merged 7 commits into
zarr-developers:masterfrom 
joshmoore:issue-840
Conversation
@joshmoore
@joshmoorejoshmoore
commented
Oct 18, 2021•

edited

Loading

Copy link


Copy Markdown

Member
fix#840

As pointed out @abred, zarr 2.10 (and earlier) was no longer properly handling the opening of nested datasets if they did not contain the dimension_separator metadata in .zarray. This is fixed with this PR. However, there are a number of cases which still xfail:
$ pytest zarr/tests/test_dim_separator.py -sv | grep XFAIL
zarr/tests/test_dim_separator.py::test_open[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_fsstore[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_directory[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[static_flat_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[directory_default] XFAIL
zarr/tests/test_dim_separator.py::test_nested[fs_default] XFAIL

Essentially for legacy datasets without the dimension_separator the following holds:
  • All nested datasets MUST be opened with NestedDirectoryStore.
  • All flat datasets MUST NOT be opened with NestedDirectoryStore.
These restrictions can be circumvented by updating your .zarray files to contain the proper metadata:
$ git grep dimension_separator fixture/{nested,flat}{,_legacy}
fixture/flat/.zarray: "dimension_separator": ".",
fixture/nested/.zarray: "dimension_separator": "/",

TODO:
  •  Add unit tests and/or doctests in docstrings
  •  Add docstrings and API docs for any new/modified user-facing classes and functions
  •  New/modified features documented in docs/tutorial.rst
  •  Changes documented in docs/release.rst
  •  GitHub Actions have all passed
  •  Test coverage is 100% (Codecov passes)
@joshmoore
Fixzarr-developers#840
c36ec3d
@joshmoore
Add legacy tests (Seezarr-developers#840)
a35a1a5
Each fixture (flat & nested) should have a version
which does not include any new metadata.
@pep8speaks
pep8speaks
commented
Oct 18, 2021•

edited

Loading

Copy link


Copy Markdown

Hello @joshmoore! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻
Comment last updated at 2021-10-19 19:48:47 UTC
@joshmoorejoshmoore
mentioned this pull request
Oct 18, 2021

Closed

@codecov
codecovBot
commented
Oct 19, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #850 (6754503) into master (4f8cb35) will increase coverage by 0.00%.

The diff coverage is 100.00%.
❗ Current head 6754503 differs from pull request most recent head ff1b9a9. Consider uploading reports for the commit ff1b9a9 to get more accurate results
@@ Coverage Diff @@## master #850 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10936 10952 +16 =======================================
+ Hits 10930 10946 +16 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/core.py100.00% <100.00%> (ø)
zarr/meta.py100.00% <100.00%> (ø)
zarr/tests/test_dim_separator.py100.00% <100.00%> (ø)
@joshmoore
joshmoore
commented
Oct 19, 2021•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'd propose to get this out as a quick 2.10.2 if there are no objections.
@joshmoore
joshmoore
merged commit d1dc987
into
zarr-developers:masterOct 19, 2021
@joshmoore
joshmoore
deleted the

issue-840

branch
October 19, 2021 19:49
@jakirkham
jakirkham
commented
Oct 19, 2021

Copy link


Copy Markdown

Member
Thanks Josh! 😄
@joshmoore
joshmoore
commented
Oct 20, 2021

Copy link


Copy Markdown

MemberAuthor
Note that one of the tests that passed here is failing in the feedstock:

1 similar comment

@joshmoore
joshmoore
commented
Oct 20, 2021

Copy link


Copy Markdown

MemberAuthor
Note that one of the tests that passed here is failing in the feedstock:
jakirkham
jakirkham
reviewed
Oct 21, 2021
View reviewed changes
Comment thread.github/workflows/minimal.yml
 run: |
 conda activate minimal
 python -m pip install -e .
 python -m pip install .
@jakirkhamjakirkhamOct 21, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, why this was needed? Did it fail without this? If so, that may be indicative of the issue we are seeing in conda-forge
@joshmoorejoshmooreOct 21, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping the editable install fixed the error "No module named 'pip'". More info in pypa/pip#10573
@jakirkhamjakirkham
mentioned this pull request
Oct 21, 2021
 Closed

3 tasks
@joshmoorejoshmoore
mentioned this pull request
Feb 9, 2022

Open

19 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jakirkhamjakirkhamjakirkham left review comments

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

zarr 2.10.0 fails to load pre-existing nested files


3 participants

@joshmoore@pep8speaks@jakirkham

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Handle legacy nested datasets (with tests) - #850
Merged
joshmoore merged 7 commits into
zarr-developers:masterfrom 
joshmoore:issue-840
Oct 19, 2021
Merged
Handle legacy nested datasets (with tests)#850
joshmoore merged 7 commits into
zarr-developers:masterfrom 
joshmoore:issue-840
Conversation
@joshmoore
@joshmoorejoshmoore
commented
Oct 18, 2021•

edited

Loading

Copy link


Copy Markdown

Member
fix#840

As pointed out @abred, zarr 2.10 (and earlier) was no longer properly handling the opening of nested datasets if they did not contain the dimension_separator metadata in .zarray. This is fixed with this PR. However, there are a number of cases which still xfail:
$ pytest zarr/tests/test_dim_separator.py -sv | grep XFAIL
zarr/tests/test_dim_separator.py::test_open[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_fsstore[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_directory[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[static_flat_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[directory_default] XFAIL
zarr/tests/test_dim_separator.py::test_nested[fs_default] XFAIL

Essentially for legacy datasets without the dimension_separator the following holds:
  • All nested datasets MUST be opened with NestedDirectoryStore.
  • All flat datasets MUST NOT be opened with NestedDirectoryStore.
These restrictions can be circumvented by updating your .zarray files to contain the proper metadata:
$ git grep dimension_separator fixture/{nested,flat}{,_legacy}
fixture/flat/.zarray: "dimension_separator": ".",
fixture/nested/.zarray: "dimension_separator": "/",

TODO:
  •  Add unit tests and/or doctests in docstrings
  •  Add docstrings and API docs for any new/modified user-facing classes and functions
  •  New/modified features documented in docs/tutorial.rst
  •  Changes documented in docs/release.rst
  •  GitHub Actions have all passed
  •  Test coverage is 100% (Codecov passes)
@joshmoore
Fixzarr-developers#840
c36ec3d
@joshmoore
Add legacy tests (Seezarr-developers#840)
a35a1a5
Each fixture (flat & nested) should have a version
which does not include any new metadata.
@pep8speaks
pep8speaks
commented
Oct 18, 2021•

edited

Loading

Copy link


Copy Markdown

Hello @joshmoore! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻
Comment last updated at 2021-10-19 19:48:47 UTC
@joshmoorejoshmoore
mentioned this pull request
Oct 18, 2021

Closed

@codecov
codecovBot
commented
Oct 19, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #850 (6754503) into master (4f8cb35) will increase coverage by 0.00%.

The diff coverage is 100.00%.
❗ Current head 6754503 differs from pull request most recent head ff1b9a9. Consider uploading reports for the commit ff1b9a9 to get more accurate results
@@ Coverage Diff @@## master #850 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10936 10952 +16 =======================================
+ Hits 10930 10946 +16 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/core.py100.00% <100.00%> (ø)
zarr/meta.py100.00% <100.00%> (ø)
zarr/tests/test_dim_separator.py100.00% <100.00%> (ø)
@joshmoore
joshmoore
commented
Oct 19, 2021•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'd propose to get this out as a quick 2.10.2 if there are no objections.
@joshmoore
joshmoore
merged commit d1dc987
into
zarr-developers:masterOct 19, 2021
@joshmoore
joshmoore
deleted the

issue-840

branch
October 19, 2021 19:49
@jakirkham
jakirkham
commented
Oct 19, 2021

Copy link


Copy Markdown

Member
Thanks Josh! 😄
@joshmoore
joshmoore
commented
Oct 20, 2021

Copy link


Copy Markdown

MemberAuthor
Note that one of the tests that passed here is failing in the feedstock:

1 similar comment

@joshmoore
joshmoore
commented
Oct 20, 2021

Copy link


Copy Markdown

MemberAuthor
Note that one of the tests that passed here is failing in the feedstock:
jakirkham
jakirkham
reviewed
Oct 21, 2021
View reviewed changes
Comment thread.github/workflows/minimal.yml
 run: |
 conda activate minimal
 python -m pip install -e .
 python -m pip install .
@jakirkhamjakirkhamOct 21, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, why this was needed? Did it fail without this? If so, that may be indicative of the issue we are seeing in conda-forge
@joshmoorejoshmooreOct 21, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping the editable install fixed the error "No module named 'pip'". More info in pypa/pip#10573
@jakirkhamjakirkham
mentioned this pull request
Oct 21, 2021
 Closed

3 tasks
@joshmoorejoshmoore
mentioned this pull request
Feb 9, 2022

Open

19 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jakirkhamjakirkhamjakirkham left review comments

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

zarr 2.10.0 fails to load pre-existing nested files


3 participants

@joshmoore@pep8speaks@jakirkham

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Handle legacy nested datasets (with tests) - #850
Merged
joshmoore merged 7 commits into
zarr-developers:masterfrom 
joshmoore:issue-840
Oct 19, 2021
Merged
Handle legacy nested datasets (with tests)#850
joshmoore merged 7 commits into
zarr-developers:masterfrom 
joshmoore:issue-840
Conversation
@joshmoore
@joshmoorejoshmoore
commented
Oct 18, 2021•

edited

Loading

Copy link


Copy Markdown

Member
fix#840

As pointed out @abred, zarr 2.10 (and earlier) was no longer properly handling the opening of nested datasets if they did not contain the dimension_separator metadata in .zarray. This is fixed with this PR. However, there are a number of cases which still xfail:
$ pytest zarr/tests/test_dim_separator.py -sv | grep XFAIL
zarr/tests/test_dim_separator.py::test_open[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_fsstore[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_directory[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[static_flat_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[directory_default] XFAIL
zarr/tests/test_dim_separator.py::test_nested[fs_default] XFAIL

Essentially for legacy datasets without the dimension_separator the following holds:
  • All nested datasets MUST be opened with NestedDirectoryStore.
  • All flat datasets MUST NOT be opened with NestedDirectoryStore.
These restrictions can be circumvented by updating your .zarray files to contain the proper metadata:
$ git grep dimension_separator fixture/{nested,flat}{,_legacy}
fixture/flat/.zarray: "dimension_separator": ".",
fixture/nested/.zarray: "dimension_separator": "/",

TODO:
  •  Add unit tests and/or doctests in docstrings
  •  Add docstrings and API docs for any new/modified user-facing classes and functions
  •  New/modified features documented in docs/tutorial.rst
  •  Changes documented in docs/release.rst
  •  GitHub Actions have all passed
  •  Test coverage is 100% (Codecov passes)
@joshmoore
Fixzarr-developers#840
c36ec3d
@joshmoore
Add legacy tests (Seezarr-developers#840)
a35a1a5
Each fixture (flat & nested) should have a version
which does not include any new metadata.
@pep8speaks
pep8speaks
commented
Oct 18, 2021•

edited

Loading

Copy link


Copy Markdown

Hello @joshmoore! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻
Comment last updated at 2021-10-19 19:48:47 UTC
@joshmoorejoshmoore
mentioned this pull request
Oct 18, 2021

Closed

@codecov
codecovBot
commented
Oct 19, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #850 (6754503) into master (4f8cb35) will increase coverage by 0.00%.

The diff coverage is 100.00%.
❗ Current head 6754503 differs from pull request most recent head ff1b9a9. Consider uploading reports for the commit ff1b9a9 to get more accurate results
@@ Coverage Diff @@## master #850 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10936 10952 +16 =======================================
+ Hits 10930 10946 +16 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/core.py100.00% <100.00%> (ø)
zarr/meta.py100.00% <100.00%> (ø)
zarr/tests/test_dim_separator.py100.00% <100.00%> (ø)
@joshmoore
joshmoore
commented
Oct 19, 2021•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'd propose to get this out as a quick 2.10.2 if there are no objections.
@joshmoore
joshmoore
merged commit d1dc987
into
zarr-developers:masterOct 19, 2021
@joshmoore
joshmoore
deleted the

issue-840

branch
October 19, 2021 19:49
@jakirkham
jakirkham
commented
Oct 19, 2021

Copy link


Copy Markdown

Member
Thanks Josh! 😄
@joshmoore
joshmoore
commented
Oct 20, 2021

Copy link


Copy Markdown

MemberAuthor
Note that one of the tests that passed here is failing in the feedstock:

1 similar comment

@joshmoore
joshmoore
commented
Oct 20, 2021

Copy link


Copy Markdown

MemberAuthor
Note that one of the tests that passed here is failing in the feedstock:
jakirkham
jakirkham
reviewed
Oct 21, 2021
View reviewed changes
Comment thread.github/workflows/minimal.yml
 run: |
 conda activate minimal
 python -m pip install -e .
 python -m pip install .
@jakirkhamjakirkhamOct 21, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, why this was needed? Did it fail without this? If so, that may be indicative of the issue we are seeing in conda-forge
@joshmoorejoshmooreOct 21, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping the editable install fixed the error "No module named 'pip'". More info in pypa/pip#10573
@jakirkhamjakirkham
mentioned this pull request
Oct 21, 2021
 Closed

3 tasks
@joshmoorejoshmoore
mentioned this pull request
Feb 9, 2022

Open

19 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jakirkhamjakirkhamjakirkham left review comments

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

zarr 2.10.0 fails to load pre-existing nested files


3 participants

@joshmoore@pep8speaks@jakirkham

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Handle legacy nested datasets (with tests) - #850
Merged
joshmoore merged 7 commits into
zarr-developers:masterfrom 
joshmoore:issue-840
Oct 19, 2021
Merged
Handle legacy nested datasets (with tests)#850
joshmoore merged 7 commits into
zarr-developers:masterfrom 
joshmoore:issue-840
Conversation
@joshmoore
@joshmoorejoshmoore
commented
Oct 18, 2021•

edited

Loading

Copy link


Copy Markdown

Member
fix#840

As pointed out @abred, zarr 2.10 (and earlier) was no longer properly handling the opening of nested datasets if they did not contain the dimension_separator metadata in .zarray. This is fixed with this PR. However, there are a number of cases which still xfail:
$ pytest zarr/tests/test_dim_separator.py -sv | grep XFAIL
zarr/tests/test_dim_separator.py::test_open[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_fsstore[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_directory[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[static_flat_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[directory_default] XFAIL
zarr/tests/test_dim_separator.py::test_nested[fs_default] XFAIL

Essentially for legacy datasets without the dimension_separator the following holds:
  • All nested datasets MUST be opened with NestedDirectoryStore.
  • All flat datasets MUST NOT be opened with NestedDirectoryStore.
These restrictions can be circumvented by updating your .zarray files to contain the proper metadata:
$ git grep dimension_separator fixture/{nested,flat}{,_legacy}
fixture/flat/.zarray: "dimension_separator": ".",
fixture/nested/.zarray: "dimension_separator": "/",

TODO:
  •  Add unit tests and/or doctests in docstrings
  •  Add docstrings and API docs for any new/modified user-facing classes and functions
  •  New/modified features documented in docs/tutorial.rst
  •  Changes documented in docs/release.rst
  •  GitHub Actions have all passed
  •  Test coverage is 100% (Codecov passes)
@joshmoore
Fixzarr-developers#840
c36ec3d
@joshmoore
Add legacy tests (Seezarr-developers#840)
a35a1a5
Each fixture (flat & nested) should have a version
which does not include any new metadata.
@pep8speaks
pep8speaks
commented
Oct 18, 2021•

edited

Loading

Copy link


Copy Markdown

Hello @joshmoore! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻
Comment last updated at 2021-10-19 19:48:47 UTC
@joshmoorejoshmoore
mentioned this pull request
Oct 18, 2021

Closed

@codecov
codecovBot
commented
Oct 19, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #850 (6754503) into master (4f8cb35) will increase coverage by 0.00%.

The diff coverage is 100.00%.
❗ Current head 6754503 differs from pull request most recent head ff1b9a9. Consider uploading reports for the commit ff1b9a9 to get more accurate results
@@ Coverage Diff @@## master #850 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10936 10952 +16 =======================================
+ Hits 10930 10946 +16 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/core.py100.00% <100.00%> (ø)
zarr/meta.py100.00% <100.00%> (ø)
zarr/tests/test_dim_separator.py100.00% <100.00%> (ø)
@joshmoore
joshmoore
commented
Oct 19, 2021•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'd propose to get this out as a quick 2.10.2 if there are no objections.
@joshmoore
joshmoore
merged commit d1dc987
into
zarr-developers:masterOct 19, 2021
@joshmoore
joshmoore
deleted the

issue-840

branch
October 19, 2021 19:49
@jakirkham
jakirkham
commented
Oct 19, 2021

Copy link


Copy Markdown

Member
Thanks Josh! 😄
@joshmoore
joshmoore
commented
Oct 20, 2021

Copy link


Copy Markdown

MemberAuthor
Note that one of the tests that passed here is failing in the feedstock:

1 similar comment

@joshmoore
joshmoore
commented
Oct 20, 2021

Copy link


Copy Markdown

MemberAuthor
Note that one of the tests that passed here is failing in the feedstock:
jakirkham
jakirkham
reviewed
Oct 21, 2021
View reviewed changes
Comment thread.github/workflows/minimal.yml
 run: |
 conda activate minimal
 python -m pip install -e .
 python -m pip install .
@jakirkhamjakirkhamOct 21, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, why this was needed? Did it fail without this? If so, that may be indicative of the issue we are seeing in conda-forge
@joshmoorejoshmooreOct 21, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping the editable install fixed the error "No module named 'pip'". More info in pypa/pip#10573
@jakirkhamjakirkham
mentioned this pull request
Oct 21, 2021
 Closed

3 tasks
@joshmoorejoshmoore
mentioned this pull request
Feb 9, 2022

Open

19 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jakirkhamjakirkhamjakirkham left review comments

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

zarr 2.10.0 fails to load pre-existing nested files


3 participants

@joshmoore@pep8speaks@jakirkham

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Handle legacy nested datasets (with tests) - #850
Merged
joshmoore merged 7 commits into
zarr-developers:masterfrom 
joshmoore:issue-840
Oct 19, 2021
Merged
Handle legacy nested datasets (with tests)#850
joshmoore merged 7 commits into
zarr-developers:masterfrom 
joshmoore:issue-840
Conversation
@joshmoore
@joshmoorejoshmoore
commented
Oct 18, 2021•

edited

Loading

Copy link


Copy Markdown

Member
fix#840

As pointed out @abred, zarr 2.10 (and earlier) was no longer properly handling the opening of nested datasets if they did not contain the dimension_separator metadata in .zarray. This is fixed with this PR. However, there are a number of cases which still xfail:
$ pytest zarr/tests/test_dim_separator.py -sv | grep XFAIL
zarr/tests/test_dim_separator.py::test_open[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_fsstore[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_directory[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[static_flat_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[directory_default] XFAIL
zarr/tests/test_dim_separator.py::test_nested[fs_default] XFAIL

Essentially for legacy datasets without the dimension_separator the following holds:
  • All nested datasets MUST be opened with NestedDirectoryStore.
  • All flat datasets MUST NOT be opened with NestedDirectoryStore.
These restrictions can be circumvented by updating your .zarray files to contain the proper metadata:
$ git grep dimension_separator fixture/{nested,flat}{,_legacy}
fixture/flat/.zarray: "dimension_separator": ".",
fixture/nested/.zarray: "dimension_separator": "/",

TODO:
  •  Add unit tests and/or doctests in docstrings
  •  Add docstrings and API docs for any new/modified user-facing classes and functions
  •  New/modified features documented in docs/tutorial.rst
  •  Changes documented in docs/release.rst
  •  GitHub Actions have all passed
  •  Test coverage is 100% (Codecov passes)
@joshmoore
Fixzarr-developers#840
c36ec3d
@joshmoore
Add legacy tests (Seezarr-developers#840)
a35a1a5
Each fixture (flat & nested) should have a version
which does not include any new metadata.
@pep8speaks
pep8speaks
commented
Oct 18, 2021•

edited

Loading

Copy link


Copy Markdown

Hello @joshmoore! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻
Comment last updated at 2021-10-19 19:48:47 UTC
@joshmoorejoshmoore
mentioned this pull request
Oct 18, 2021

Closed

@codecov
codecovBot
commented
Oct 19, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #850 (6754503) into master (4f8cb35) will increase coverage by 0.00%.

The diff coverage is 100.00%.
❗ Current head 6754503 differs from pull request most recent head ff1b9a9. Consider uploading reports for the commit ff1b9a9 to get more accurate results
@@ Coverage Diff @@## master #850 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10936 10952 +16 =======================================
+ Hits 10930 10946 +16 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/core.py100.00% <100.00%> (ø)
zarr/meta.py100.00% <100.00%> (ø)
zarr/tests/test_dim_separator.py100.00% <100.00%> (ø)
@joshmoore
joshmoore
commented
Oct 19, 2021•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'd propose to get this out as a quick 2.10.2 if there are no objections.
@joshmoore
joshmoore
merged commit d1dc987
into
zarr-developers:masterOct 19, 2021
@joshmoore
joshmoore
deleted the

issue-840

branch
October 19, 2021 19:49
@jakirkham
jakirkham
commented
Oct 19, 2021

Copy link


Copy Markdown

Member
Thanks Josh! 😄
@joshmoore
joshmoore
commented
Oct 20, 2021

Copy link


Copy Markdown

MemberAuthor
Note that one of the tests that passed here is failing in the feedstock:

1 similar comment

@joshmoore
joshmoore
commented
Oct 20, 2021

Copy link


Copy Markdown

MemberAuthor
Note that one of the tests that passed here is failing in the feedstock:
jakirkham
jakirkham
reviewed
Oct 21, 2021
View reviewed changes
Comment thread.github/workflows/minimal.yml
 run: |
 conda activate minimal
 python -m pip install -e .
 python -m pip install .
@jakirkhamjakirkhamOct 21, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, why this was needed? Did it fail without this? If so, that may be indicative of the issue we are seeing in conda-forge
@joshmoorejoshmooreOct 21, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping the editable install fixed the error "No module named 'pip'". More info in pypa/pip#10573
@jakirkhamjakirkham
mentioned this pull request
Oct 21, 2021
 Closed

3 tasks
@joshmoorejoshmoore
mentioned this pull request
Feb 9, 2022

Open

19 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jakirkhamjakirkhamjakirkham left review comments

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

zarr 2.10.0 fails to load pre-existing nested files


3 participants

@joshmoore@pep8speaks@jakirkham

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Handle legacy nested datasets (with tests) - #850
Merged
joshmoore merged 7 commits into
zarr-developers:masterfrom 
joshmoore:issue-840
Oct 19, 2021
Merged
Handle legacy nested datasets (with tests)#850
joshmoore merged 7 commits into
zarr-developers:masterfrom 
joshmoore:issue-840
Conversation
@joshmoore
@joshmoorejoshmoore
commented
Oct 18, 2021•

edited

Loading

Copy link


Copy Markdown

Member
fix#840

As pointed out @abred, zarr 2.10 (and earlier) was no longer properly handling the opening of nested datasets if they did not contain the dimension_separator metadata in .zarray. This is fixed with this PR. However, there are a number of cases which still xfail:
$ pytest zarr/tests/test_dim_separator.py -sv | grep XFAIL
zarr/tests/test_dim_separator.py::test_open[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_fsstore[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_directory[static_nested_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[static_flat_legacy] XFAIL
zarr/tests/test_dim_separator.py::test_nested[directory_default] XFAIL
zarr/tests/test_dim_separator.py::test_nested[fs_default] XFAIL

Essentially for legacy datasets without the dimension_separator the following holds:
  • All nested datasets MUST be opened with NestedDirectoryStore.
  • All flat datasets MUST NOT be opened with NestedDirectoryStore.
These restrictions can be circumvented by updating your .zarray files to contain the proper metadata:
$ git grep dimension_separator fixture/{nested,flat}{,_legacy}
fixture/flat/.zarray: "dimension_separator": ".",
fixture/nested/.zarray: "dimension_separator": "/",

TODO:
  •  Add unit tests and/or doctests in docstrings
  •  Add docstrings and API docs for any new/modified user-facing classes and functions
  •  New/modified features documented in docs/tutorial.rst
  •  Changes documented in docs/release.rst
  •  GitHub Actions have all passed
  •  Test coverage is 100% (Codecov passes)
@joshmoore
Fixzarr-developers#840
c36ec3d
@joshmoore
Add legacy tests (Seezarr-developers#840)
a35a1a5
Each fixture (flat & nested) should have a version
which does not include any new metadata.
@pep8speaks
pep8speaks
commented
Oct 18, 2021•

edited

Loading

Copy link


Copy Markdown

Hello @joshmoore! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻
Comment last updated at 2021-10-19 19:48:47 UTC
@joshmoorejoshmoore
mentioned this pull request
Oct 18, 2021

Closed

@codecov
codecovBot
commented
Oct 19, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #850 (6754503) into master (4f8cb35) will increase coverage by 0.00%.

The diff coverage is 100.00%.
❗ Current head 6754503 differs from pull request most recent head ff1b9a9. Consider uploading reports for the commit ff1b9a9 to get more accurate results
@@ Coverage Diff @@## master #850 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10936 10952 +16 =======================================
+ Hits 10930 10946 +16 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/core.py100.00% <100.00%> (ø)
zarr/meta.py100.00% <100.00%> (ø)
zarr/tests/test_dim_separator.py100.00% <100.00%> (ø)
@joshmoore
joshmoore
commented
Oct 19, 2021•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'd propose to get this out as a quick 2.10.2 if there are no objections.
@joshmoore
joshmoore
merged commit d1dc987
into
zarr-developers:masterOct 19, 2021
@joshmoore
joshmoore
deleted the

issue-840

branch
October 19, 2021 19:49
@jakirkham
jakirkham
commented
Oct 19, 2021

Copy link


Copy Markdown

Member
Thanks Josh! 😄
@joshmoore
joshmoore
commented
Oct 20, 2021

Copy link


Copy Markdown

MemberAuthor
Note that one of the tests that passed here is failing in the feedstock:

1 similar comment

@joshmoore
joshmoore
commented
Oct 20, 2021

Copy link


Copy Markdown

MemberAuthor
Note that one of the tests that passed here is failing in the feedstock:
jakirkham
jakirkham
reviewed
Oct 21, 2021
View reviewed changes
Comment thread.github/workflows/minimal.yml
 run: |
 conda activate minimal
 python -m pip install -e .
 python -m pip install .
@jakirkhamjakirkhamOct 21, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, why this was needed? Did it fail without this? If so, that may be indicative of the issue we are seeing in conda-forge
@joshmoorejoshmooreOct 21, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping the editable install fixed the error "No module named 'pip'". More info in pypa/pip#10573
@jakirkhamjakirkham
mentioned this pull request
Oct 21, 2021
 Closed

3 tasks
@joshmoorejoshmoore
mentioned this pull request
Feb 9, 2022

Open

19 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jakirkhamjakirkhamjakirkham left review comments

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

zarr 2.10.0 fails to load pre-existing nested files


3 participants

@joshmoore@pep8speaks@jakirkham