Resolve Mypy erorrs in v3 branch - #1692

Merged
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy
Apr 6, 2024
Merged

Resolve Mypy erorrs in v3 branch#1692
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy

Conversation

@DahnJ

@DahnJDahnJ commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

Attempts to address #1593

There are still two unsolved issues around the nonexistant Store.from_path in core.py.

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)

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Not solved here, as per comment. What subclass of Store should this use?

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

@DahnJDahnJ mentioned this pull request Mar 4, 2024
2 tasks
Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Similar here

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.

See above.

Comment threadsrc/zarr/v3/sync.py
def _sync_iter(
self, func: Callable[P, AsyncIterator[T]], *args: P.args, **kwargs: P.kwargs
) -> List[T]:
def _sync_iter(self, coroutine: Coroutine[Any, Any, AsyncIterator[T]]) -> List[T]:

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I removed the args/kwargs here to make it handle a coroutine, which is how it's so far being used.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @DahnJ. I've launched the workflows.

@DahnJ

DahnJ commented Mar 5, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @joshmoore I tried to fix a failing test (which I think happened because Any was used in a Pydantic model), can you launch them again please?

@joshmoore

Copy link
Copy Markdown
Member

On their way! 🏇🏽

@jhammanjhamman left a comment

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.

Thanks @DahnJ for jumping in here! Really happy to see this contribution.

Some guiding principles that may help the work here progress. First, I would prioritize getting Mypy to pass, even if it requires ignoring a few problematic lines. Then, once things are passing again (see #1649 where we disable the mypy checks in the v3 CI), then we can come back and fix any TODOs.

Comment threadsrc/zarr/v3/group.py Outdated
else:
return {
ZGROUP_JSON: self.zarr_format,
ZGROUP_JSON: str(self.zarr_format).encode(),

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.

Flagging that this was actually a bug in hiding. I believe it should have been something like:

Suggested change
ZGROUP_JSON: str(self.zarr_format).encode(),
ZGROUP_JSON: json.dumps({"zarr_format": 2}).encode(),

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Addressed in a30af88

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

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.

See above.

@DahnJ

Copy link
Copy Markdown
ContributorAuthor

@jhamman I have addressed the comments in a30af88

I was not sure how to handle the secondStore.from_path() reference. What should happen if the store_like argument is of type str? I tried to make a potential suggestion based on a commented-out code in f57527a. Let me please know if this is the correct way of handling that case.

As for getting mypy to pass, it does pass for me locally and I didn't add any type: ignore lines. However, there already are a few in zarr/v3 – I removed some more in e1fdd1e and then turned on mypy in pre-commit in db6da34.

@jhamman

Copy link
Copy Markdown
Member

@DahnJ - I think this is a great step forward. I want @d-v-b to review this but from my perspective, it could go in now, even with the outstanding store issue.

@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

This looks good! There's some overlap with stuff I did over in #1743, but that's not a problem. I will merge and rebase #1743 on this.

@d-v-b
d-v-b merged commit 15a9747 into zarr-developers:v3Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

thanks @DahnJ !

d-v-b pushed a commit to d-v-b/zarr-python that referenced this pull request Apr 10, 2024
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
@d-v-bd-v-b mentioned this pull request Apr 12, 2024
6 tasks
d-v-b added a commit that referenced this pull request Apr 22, 2024
* chore: add deprecation warnings to v3 classes / functions
* Resolve Mypy erorrs in `v3` branch (#1692)
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
* Specify hatch envs using GitHub actions matrix for v3 tests (#1728)
* Specify v3 hatch envs using GitHub actions matrix
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* test on 3.12 too
* no 3.12
---------
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
* black -> ruff format + cleanup (#1639)
* black -> ruff + cleanup
* format
* Preserve git blame
* pre-commit fix
* Remove outdated dev install docs from installation.rst and link to contributing.rst (#1643)
Co-authored-by: Joe Hamman <joe@earthmover.io>
* chore: remove old v3 implementation
* chore: remove more version-conditional logic
* chore: remove v3_storage_transformers.py again
---------
Co-authored-by: Daniel Jahn (dahn) <dahnjahn@gmail.com>
Co-authored-by: Max Jones <14077947+maxrjones@users.noreply.github.com>
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
Co-authored-by: Alden Keefe Sampson <aldenkeefesampson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@DahnJ@joshmoore@jhamman@d-v-b@sanketverma1704
, '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

Resolve Mypy erorrs in v3 branch - #1692

Merged
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy
Apr 6, 2024
Merged

Resolve Mypy erorrs in v3 branch#1692
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy

Conversation

@DahnJ

@DahnJDahnJ commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

Attempts to address #1593

There are still two unsolved issues around the nonexistant Store.from_path in core.py.

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)

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Not solved here, as per comment. What subclass of Store should this use?

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

@DahnJDahnJ mentioned this pull request Mar 4, 2024
2 tasks
Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Similar here

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.

See above.

Comment threadsrc/zarr/v3/sync.py
def _sync_iter(
self, func: Callable[P, AsyncIterator[T]], *args: P.args, **kwargs: P.kwargs
) -> List[T]:
def _sync_iter(self, coroutine: Coroutine[Any, Any, AsyncIterator[T]]) -> List[T]:

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I removed the args/kwargs here to make it handle a coroutine, which is how it's so far being used.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @DahnJ. I've launched the workflows.

@DahnJ

DahnJ commented Mar 5, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @joshmoore I tried to fix a failing test (which I think happened because Any was used in a Pydantic model), can you launch them again please?

@joshmoore

Copy link
Copy Markdown
Member

On their way! 🏇🏽

@jhammanjhamman left a comment

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.

Thanks @DahnJ for jumping in here! Really happy to see this contribution.

Some guiding principles that may help the work here progress. First, I would prioritize getting Mypy to pass, even if it requires ignoring a few problematic lines. Then, once things are passing again (see #1649 where we disable the mypy checks in the v3 CI), then we can come back and fix any TODOs.

Comment threadsrc/zarr/v3/group.py Outdated
else:
return {
ZGROUP_JSON: self.zarr_format,
ZGROUP_JSON: str(self.zarr_format).encode(),

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.

Flagging that this was actually a bug in hiding. I believe it should have been something like:

Suggested change
ZGROUP_JSON: str(self.zarr_format).encode(),
ZGROUP_JSON: json.dumps({"zarr_format": 2}).encode(),

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Addressed in a30af88

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

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.

See above.

@DahnJ

Copy link
Copy Markdown
ContributorAuthor

@jhamman I have addressed the comments in a30af88

I was not sure how to handle the secondStore.from_path() reference. What should happen if the store_like argument is of type str? I tried to make a potential suggestion based on a commented-out code in f57527a. Let me please know if this is the correct way of handling that case.

As for getting mypy to pass, it does pass for me locally and I didn't add any type: ignore lines. However, there already are a few in zarr/v3 – I removed some more in e1fdd1e and then turned on mypy in pre-commit in db6da34.

@jhamman

Copy link
Copy Markdown
Member

@DahnJ - I think this is a great step forward. I want @d-v-b to review this but from my perspective, it could go in now, even with the outstanding store issue.

@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

This looks good! There's some overlap with stuff I did over in #1743, but that's not a problem. I will merge and rebase #1743 on this.

@d-v-b
d-v-b merged commit 15a9747 into zarr-developers:v3Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

thanks @DahnJ !

d-v-b pushed a commit to d-v-b/zarr-python that referenced this pull request Apr 10, 2024
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
@d-v-bd-v-b mentioned this pull request Apr 12, 2024
6 tasks
d-v-b added a commit that referenced this pull request Apr 22, 2024
* chore: add deprecation warnings to v3 classes / functions
* Resolve Mypy erorrs in `v3` branch (#1692)
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
* Specify hatch envs using GitHub actions matrix for v3 tests (#1728)
* Specify v3 hatch envs using GitHub actions matrix
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* test on 3.12 too
* no 3.12
---------
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
* black -> ruff format + cleanup (#1639)
* black -> ruff + cleanup
* format
* Preserve git blame
* pre-commit fix
* Remove outdated dev install docs from installation.rst and link to contributing.rst (#1643)
Co-authored-by: Joe Hamman <joe@earthmover.io>
* chore: remove old v3 implementation
* chore: remove more version-conditional logic
* chore: remove v3_storage_transformers.py again
---------
Co-authored-by: Daniel Jahn (dahn) <dahnjahn@gmail.com>
Co-authored-by: Max Jones <14077947+maxrjones@users.noreply.github.com>
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
Co-authored-by: Alden Keefe Sampson <aldenkeefesampson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@DahnJ@joshmoore@jhamman@d-v-b@sanketverma1704
, '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

Resolve Mypy erorrs in v3 branch - #1692

Merged
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy
Apr 6, 2024
Merged

Resolve Mypy erorrs in v3 branch#1692
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy

Conversation

@DahnJ

@DahnJDahnJ commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

Attempts to address #1593

There are still two unsolved issues around the nonexistant Store.from_path in core.py.

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)

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Not solved here, as per comment. What subclass of Store should this use?

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

@DahnJDahnJ mentioned this pull request Mar 4, 2024
2 tasks
Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Similar here

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.

See above.

Comment threadsrc/zarr/v3/sync.py
def _sync_iter(
self, func: Callable[P, AsyncIterator[T]], *args: P.args, **kwargs: P.kwargs
) -> List[T]:
def _sync_iter(self, coroutine: Coroutine[Any, Any, AsyncIterator[T]]) -> List[T]:

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I removed the args/kwargs here to make it handle a coroutine, which is how it's so far being used.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @DahnJ. I've launched the workflows.

@DahnJ

DahnJ commented Mar 5, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @joshmoore I tried to fix a failing test (which I think happened because Any was used in a Pydantic model), can you launch them again please?

@joshmoore

Copy link
Copy Markdown
Member

On their way! 🏇🏽

@jhammanjhamman left a comment

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.

Thanks @DahnJ for jumping in here! Really happy to see this contribution.

Some guiding principles that may help the work here progress. First, I would prioritize getting Mypy to pass, even if it requires ignoring a few problematic lines. Then, once things are passing again (see #1649 where we disable the mypy checks in the v3 CI), then we can come back and fix any TODOs.

Comment threadsrc/zarr/v3/group.py Outdated
else:
return {
ZGROUP_JSON: self.zarr_format,
ZGROUP_JSON: str(self.zarr_format).encode(),

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.

Flagging that this was actually a bug in hiding. I believe it should have been something like:

Suggested change
ZGROUP_JSON: str(self.zarr_format).encode(),
ZGROUP_JSON: json.dumps({"zarr_format": 2}).encode(),

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Addressed in a30af88

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

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.

See above.

@DahnJ

Copy link
Copy Markdown
ContributorAuthor

@jhamman I have addressed the comments in a30af88

I was not sure how to handle the secondStore.from_path() reference. What should happen if the store_like argument is of type str? I tried to make a potential suggestion based on a commented-out code in f57527a. Let me please know if this is the correct way of handling that case.

As for getting mypy to pass, it does pass for me locally and I didn't add any type: ignore lines. However, there already are a few in zarr/v3 – I removed some more in e1fdd1e and then turned on mypy in pre-commit in db6da34.

@jhamman

Copy link
Copy Markdown
Member

@DahnJ - I think this is a great step forward. I want @d-v-b to review this but from my perspective, it could go in now, even with the outstanding store issue.

@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

This looks good! There's some overlap with stuff I did over in #1743, but that's not a problem. I will merge and rebase #1743 on this.

@d-v-b
d-v-b merged commit 15a9747 into zarr-developers:v3Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

thanks @DahnJ !

d-v-b pushed a commit to d-v-b/zarr-python that referenced this pull request Apr 10, 2024
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
@d-v-bd-v-b mentioned this pull request Apr 12, 2024
6 tasks
d-v-b added a commit that referenced this pull request Apr 22, 2024
* chore: add deprecation warnings to v3 classes / functions
* Resolve Mypy erorrs in `v3` branch (#1692)
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
* Specify hatch envs using GitHub actions matrix for v3 tests (#1728)
* Specify v3 hatch envs using GitHub actions matrix
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* test on 3.12 too
* no 3.12
---------
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
* black -> ruff format + cleanup (#1639)
* black -> ruff + cleanup
* format
* Preserve git blame
* pre-commit fix
* Remove outdated dev install docs from installation.rst and link to contributing.rst (#1643)
Co-authored-by: Joe Hamman <joe@earthmover.io>
* chore: remove old v3 implementation
* chore: remove more version-conditional logic
* chore: remove v3_storage_transformers.py again
---------
Co-authored-by: Daniel Jahn (dahn) <dahnjahn@gmail.com>
Co-authored-by: Max Jones <14077947+maxrjones@users.noreply.github.com>
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
Co-authored-by: Alden Keefe Sampson <aldenkeefesampson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@DahnJ@joshmoore@jhamman@d-v-b@sanketverma1704
, '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

Resolve Mypy erorrs in v3 branch - #1692

Merged
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy
Apr 6, 2024
Merged

Resolve Mypy erorrs in v3 branch#1692
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy

Conversation

@DahnJ

@DahnJDahnJ commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

Attempts to address #1593

There are still two unsolved issues around the nonexistant Store.from_path in core.py.

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)

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Not solved here, as per comment. What subclass of Store should this use?

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

@DahnJDahnJ mentioned this pull request Mar 4, 2024
2 tasks
Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Similar here

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.

See above.

Comment threadsrc/zarr/v3/sync.py
def _sync_iter(
self, func: Callable[P, AsyncIterator[T]], *args: P.args, **kwargs: P.kwargs
) -> List[T]:
def _sync_iter(self, coroutine: Coroutine[Any, Any, AsyncIterator[T]]) -> List[T]:

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I removed the args/kwargs here to make it handle a coroutine, which is how it's so far being used.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @DahnJ. I've launched the workflows.

@DahnJ

DahnJ commented Mar 5, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @joshmoore I tried to fix a failing test (which I think happened because Any was used in a Pydantic model), can you launch them again please?

@joshmoore

Copy link
Copy Markdown
Member

On their way! 🏇🏽

@jhammanjhamman left a comment

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.

Thanks @DahnJ for jumping in here! Really happy to see this contribution.

Some guiding principles that may help the work here progress. First, I would prioritize getting Mypy to pass, even if it requires ignoring a few problematic lines. Then, once things are passing again (see #1649 where we disable the mypy checks in the v3 CI), then we can come back and fix any TODOs.

Comment threadsrc/zarr/v3/group.py Outdated
else:
return {
ZGROUP_JSON: self.zarr_format,
ZGROUP_JSON: str(self.zarr_format).encode(),

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.

Flagging that this was actually a bug in hiding. I believe it should have been something like:

Suggested change
ZGROUP_JSON: str(self.zarr_format).encode(),
ZGROUP_JSON: json.dumps({"zarr_format": 2}).encode(),

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Addressed in a30af88

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

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.

See above.

@DahnJ

Copy link
Copy Markdown
ContributorAuthor

@jhamman I have addressed the comments in a30af88

I was not sure how to handle the secondStore.from_path() reference. What should happen if the store_like argument is of type str? I tried to make a potential suggestion based on a commented-out code in f57527a. Let me please know if this is the correct way of handling that case.

As for getting mypy to pass, it does pass for me locally and I didn't add any type: ignore lines. However, there already are a few in zarr/v3 – I removed some more in e1fdd1e and then turned on mypy in pre-commit in db6da34.

@jhamman

Copy link
Copy Markdown
Member

@DahnJ - I think this is a great step forward. I want @d-v-b to review this but from my perspective, it could go in now, even with the outstanding store issue.

@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

This looks good! There's some overlap with stuff I did over in #1743, but that's not a problem. I will merge and rebase #1743 on this.

@d-v-b
d-v-b merged commit 15a9747 into zarr-developers:v3Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

thanks @DahnJ !

d-v-b pushed a commit to d-v-b/zarr-python that referenced this pull request Apr 10, 2024
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
@d-v-bd-v-b mentioned this pull request Apr 12, 2024
6 tasks
d-v-b added a commit that referenced this pull request Apr 22, 2024
* chore: add deprecation warnings to v3 classes / functions
* Resolve Mypy erorrs in `v3` branch (#1692)
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
* Specify hatch envs using GitHub actions matrix for v3 tests (#1728)
* Specify v3 hatch envs using GitHub actions matrix
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* test on 3.12 too
* no 3.12
---------
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
* black -> ruff format + cleanup (#1639)
* black -> ruff + cleanup
* format
* Preserve git blame
* pre-commit fix
* Remove outdated dev install docs from installation.rst and link to contributing.rst (#1643)
Co-authored-by: Joe Hamman <joe@earthmover.io>
* chore: remove old v3 implementation
* chore: remove more version-conditional logic
* chore: remove v3_storage_transformers.py again
---------
Co-authored-by: Daniel Jahn (dahn) <dahnjahn@gmail.com>
Co-authored-by: Max Jones <14077947+maxrjones@users.noreply.github.com>
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
Co-authored-by: Alden Keefe Sampson <aldenkeefesampson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@DahnJ@joshmoore@jhamman@d-v-b@sanketverma1704
, '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

Resolve Mypy erorrs in v3 branch - #1692

Merged
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy
Apr 6, 2024
Merged

Resolve Mypy erorrs in v3 branch#1692
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy

Conversation

@DahnJ

@DahnJDahnJ commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

Attempts to address #1593

There are still two unsolved issues around the nonexistant Store.from_path in core.py.

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)

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Not solved here, as per comment. What subclass of Store should this use?

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

@DahnJDahnJ mentioned this pull request Mar 4, 2024
2 tasks
Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Similar here

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.

See above.

Comment threadsrc/zarr/v3/sync.py
def _sync_iter(
self, func: Callable[P, AsyncIterator[T]], *args: P.args, **kwargs: P.kwargs
) -> List[T]:
def _sync_iter(self, coroutine: Coroutine[Any, Any, AsyncIterator[T]]) -> List[T]:

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I removed the args/kwargs here to make it handle a coroutine, which is how it's so far being used.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @DahnJ. I've launched the workflows.

@DahnJ

DahnJ commented Mar 5, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @joshmoore I tried to fix a failing test (which I think happened because Any was used in a Pydantic model), can you launch them again please?

@joshmoore

Copy link
Copy Markdown
Member

On their way! 🏇🏽

@jhammanjhamman left a comment

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.

Thanks @DahnJ for jumping in here! Really happy to see this contribution.

Some guiding principles that may help the work here progress. First, I would prioritize getting Mypy to pass, even if it requires ignoring a few problematic lines. Then, once things are passing again (see #1649 where we disable the mypy checks in the v3 CI), then we can come back and fix any TODOs.

Comment threadsrc/zarr/v3/group.py Outdated
else:
return {
ZGROUP_JSON: self.zarr_format,
ZGROUP_JSON: str(self.zarr_format).encode(),

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.

Flagging that this was actually a bug in hiding. I believe it should have been something like:

Suggested change
ZGROUP_JSON: str(self.zarr_format).encode(),
ZGROUP_JSON: json.dumps({"zarr_format": 2}).encode(),

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Addressed in a30af88

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

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.

See above.

@DahnJ

Copy link
Copy Markdown
ContributorAuthor

@jhamman I have addressed the comments in a30af88

I was not sure how to handle the secondStore.from_path() reference. What should happen if the store_like argument is of type str? I tried to make a potential suggestion based on a commented-out code in f57527a. Let me please know if this is the correct way of handling that case.

As for getting mypy to pass, it does pass for me locally and I didn't add any type: ignore lines. However, there already are a few in zarr/v3 – I removed some more in e1fdd1e and then turned on mypy in pre-commit in db6da34.

@jhamman

Copy link
Copy Markdown
Member

@DahnJ - I think this is a great step forward. I want @d-v-b to review this but from my perspective, it could go in now, even with the outstanding store issue.

@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

This looks good! There's some overlap with stuff I did over in #1743, but that's not a problem. I will merge and rebase #1743 on this.

@d-v-b
d-v-b merged commit 15a9747 into zarr-developers:v3Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

thanks @DahnJ !

d-v-b pushed a commit to d-v-b/zarr-python that referenced this pull request Apr 10, 2024
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
@d-v-bd-v-b mentioned this pull request Apr 12, 2024
6 tasks
d-v-b added a commit that referenced this pull request Apr 22, 2024
* chore: add deprecation warnings to v3 classes / functions
* Resolve Mypy erorrs in `v3` branch (#1692)
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
* Specify hatch envs using GitHub actions matrix for v3 tests (#1728)
* Specify v3 hatch envs using GitHub actions matrix
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* test on 3.12 too
* no 3.12
---------
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
* black -> ruff format + cleanup (#1639)
* black -> ruff + cleanup
* format
* Preserve git blame
* pre-commit fix
* Remove outdated dev install docs from installation.rst and link to contributing.rst (#1643)
Co-authored-by: Joe Hamman <joe@earthmover.io>
* chore: remove old v3 implementation
* chore: remove more version-conditional logic
* chore: remove v3_storage_transformers.py again
---------
Co-authored-by: Daniel Jahn (dahn) <dahnjahn@gmail.com>
Co-authored-by: Max Jones <14077947+maxrjones@users.noreply.github.com>
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
Co-authored-by: Alden Keefe Sampson <aldenkeefesampson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@DahnJ@joshmoore@jhamman@d-v-b@sanketverma1704
, '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

Resolve Mypy erorrs in v3 branch - #1692

Merged
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy
Apr 6, 2024
Merged

Resolve Mypy erorrs in v3 branch#1692
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy

Conversation

@DahnJ

@DahnJDahnJ commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

Attempts to address #1593

There are still two unsolved issues around the nonexistant Store.from_path in core.py.

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)

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Not solved here, as per comment. What subclass of Store should this use?

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

@DahnJDahnJ mentioned this pull request Mar 4, 2024
2 tasks
Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Similar here

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.

See above.

Comment threadsrc/zarr/v3/sync.py
def _sync_iter(
self, func: Callable[P, AsyncIterator[T]], *args: P.args, **kwargs: P.kwargs
) -> List[T]:
def _sync_iter(self, coroutine: Coroutine[Any, Any, AsyncIterator[T]]) -> List[T]:

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I removed the args/kwargs here to make it handle a coroutine, which is how it's so far being used.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @DahnJ. I've launched the workflows.

@DahnJ

DahnJ commented Mar 5, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @joshmoore I tried to fix a failing test (which I think happened because Any was used in a Pydantic model), can you launch them again please?

@joshmoore

Copy link
Copy Markdown
Member

On their way! 🏇🏽

@jhammanjhamman left a comment

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.

Thanks @DahnJ for jumping in here! Really happy to see this contribution.

Some guiding principles that may help the work here progress. First, I would prioritize getting Mypy to pass, even if it requires ignoring a few problematic lines. Then, once things are passing again (see #1649 where we disable the mypy checks in the v3 CI), then we can come back and fix any TODOs.

Comment threadsrc/zarr/v3/group.py Outdated
else:
return {
ZGROUP_JSON: self.zarr_format,
ZGROUP_JSON: str(self.zarr_format).encode(),

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.

Flagging that this was actually a bug in hiding. I believe it should have been something like:

Suggested change
ZGROUP_JSON: str(self.zarr_format).encode(),
ZGROUP_JSON: json.dumps({"zarr_format": 2}).encode(),

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Addressed in a30af88

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

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.

See above.

@DahnJ

Copy link
Copy Markdown
ContributorAuthor

@jhamman I have addressed the comments in a30af88

I was not sure how to handle the secondStore.from_path() reference. What should happen if the store_like argument is of type str? I tried to make a potential suggestion based on a commented-out code in f57527a. Let me please know if this is the correct way of handling that case.

As for getting mypy to pass, it does pass for me locally and I didn't add any type: ignore lines. However, there already are a few in zarr/v3 – I removed some more in e1fdd1e and then turned on mypy in pre-commit in db6da34.

@jhamman

Copy link
Copy Markdown
Member

@DahnJ - I think this is a great step forward. I want @d-v-b to review this but from my perspective, it could go in now, even with the outstanding store issue.

@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

This looks good! There's some overlap with stuff I did over in #1743, but that's not a problem. I will merge and rebase #1743 on this.

@d-v-b
d-v-b merged commit 15a9747 into zarr-developers:v3Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

thanks @DahnJ !

d-v-b pushed a commit to d-v-b/zarr-python that referenced this pull request Apr 10, 2024
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
@d-v-bd-v-b mentioned this pull request Apr 12, 2024
6 tasks
d-v-b added a commit that referenced this pull request Apr 22, 2024
* chore: add deprecation warnings to v3 classes / functions
* Resolve Mypy erorrs in `v3` branch (#1692)
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
* Specify hatch envs using GitHub actions matrix for v3 tests (#1728)
* Specify v3 hatch envs using GitHub actions matrix
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* test on 3.12 too
* no 3.12
---------
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
* black -> ruff format + cleanup (#1639)
* black -> ruff + cleanup
* format
* Preserve git blame
* pre-commit fix
* Remove outdated dev install docs from installation.rst and link to contributing.rst (#1643)
Co-authored-by: Joe Hamman <joe@earthmover.io>
* chore: remove old v3 implementation
* chore: remove more version-conditional logic
* chore: remove v3_storage_transformers.py again
---------
Co-authored-by: Daniel Jahn (dahn) <dahnjahn@gmail.com>
Co-authored-by: Max Jones <14077947+maxrjones@users.noreply.github.com>
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
Co-authored-by: Alden Keefe Sampson <aldenkeefesampson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@DahnJ@joshmoore@jhamman@d-v-b@sanketverma1704
, '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

Resolve Mypy erorrs in v3 branch - #1692

Merged
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy
Apr 6, 2024
Merged

Resolve Mypy erorrs in v3 branch#1692
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy

Conversation

@DahnJ

@DahnJDahnJ commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

Attempts to address #1593

There are still two unsolved issues around the nonexistant Store.from_path in core.py.

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)

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Not solved here, as per comment. What subclass of Store should this use?

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

@DahnJDahnJ mentioned this pull request Mar 4, 2024
2 tasks
Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Similar here

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.

See above.

Comment threadsrc/zarr/v3/sync.py
def _sync_iter(
self, func: Callable[P, AsyncIterator[T]], *args: P.args, **kwargs: P.kwargs
) -> List[T]:
def _sync_iter(self, coroutine: Coroutine[Any, Any, AsyncIterator[T]]) -> List[T]:

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I removed the args/kwargs here to make it handle a coroutine, which is how it's so far being used.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @DahnJ. I've launched the workflows.

@DahnJ

DahnJ commented Mar 5, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @joshmoore I tried to fix a failing test (which I think happened because Any was used in a Pydantic model), can you launch them again please?

@joshmoore

Copy link
Copy Markdown
Member

On their way! 🏇🏽

@jhammanjhamman left a comment

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.

Thanks @DahnJ for jumping in here! Really happy to see this contribution.

Some guiding principles that may help the work here progress. First, I would prioritize getting Mypy to pass, even if it requires ignoring a few problematic lines. Then, once things are passing again (see #1649 where we disable the mypy checks in the v3 CI), then we can come back and fix any TODOs.

Comment threadsrc/zarr/v3/group.py Outdated
else:
return {
ZGROUP_JSON: self.zarr_format,
ZGROUP_JSON: str(self.zarr_format).encode(),

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.

Flagging that this was actually a bug in hiding. I believe it should have been something like:

Suggested change
ZGROUP_JSON: str(self.zarr_format).encode(),
ZGROUP_JSON: json.dumps({"zarr_format": 2}).encode(),

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Addressed in a30af88

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

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.

See above.

@DahnJ

Copy link
Copy Markdown
ContributorAuthor

@jhamman I have addressed the comments in a30af88

I was not sure how to handle the secondStore.from_path() reference. What should happen if the store_like argument is of type str? I tried to make a potential suggestion based on a commented-out code in f57527a. Let me please know if this is the correct way of handling that case.

As for getting mypy to pass, it does pass for me locally and I didn't add any type: ignore lines. However, there already are a few in zarr/v3 – I removed some more in e1fdd1e and then turned on mypy in pre-commit in db6da34.

@jhamman

Copy link
Copy Markdown
Member

@DahnJ - I think this is a great step forward. I want @d-v-b to review this but from my perspective, it could go in now, even with the outstanding store issue.

@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

This looks good! There's some overlap with stuff I did over in #1743, but that's not a problem. I will merge and rebase #1743 on this.

@d-v-b
d-v-b merged commit 15a9747 into zarr-developers:v3Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

thanks @DahnJ !

d-v-b pushed a commit to d-v-b/zarr-python that referenced this pull request Apr 10, 2024
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
@d-v-bd-v-b mentioned this pull request Apr 12, 2024
6 tasks
d-v-b added a commit that referenced this pull request Apr 22, 2024
* chore: add deprecation warnings to v3 classes / functions
* Resolve Mypy erorrs in `v3` branch (#1692)
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
* Specify hatch envs using GitHub actions matrix for v3 tests (#1728)
* Specify v3 hatch envs using GitHub actions matrix
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* test on 3.12 too
* no 3.12
---------
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
* black -> ruff format + cleanup (#1639)
* black -> ruff + cleanup
* format
* Preserve git blame
* pre-commit fix
* Remove outdated dev install docs from installation.rst and link to contributing.rst (#1643)
Co-authored-by: Joe Hamman <joe@earthmover.io>
* chore: remove old v3 implementation
* chore: remove more version-conditional logic
* chore: remove v3_storage_transformers.py again
---------
Co-authored-by: Daniel Jahn (dahn) <dahnjahn@gmail.com>
Co-authored-by: Max Jones <14077947+maxrjones@users.noreply.github.com>
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
Co-authored-by: Alden Keefe Sampson <aldenkeefesampson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@DahnJ@joshmoore@jhamman@d-v-b@sanketverma1704
, '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

Resolve Mypy erorrs in v3 branch - #1692

Merged
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy
Apr 6, 2024
Merged

Resolve Mypy erorrs in v3 branch#1692
d-v-b merged 9 commits into
zarr-developers:v3from
DahnJ:feat/v3-mypy

Conversation

@DahnJ

@DahnJDahnJ commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

Attempts to address #1593

There are still two unsolved issues around the nonexistant Store.from_path in core.py.

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)

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Not solved here, as per comment. What subclass of Store should this use?

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

@DahnJDahnJ mentioned this pull request Mar 4, 2024
2 tasks
Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Similar here

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.

See above.

Comment threadsrc/zarr/v3/sync.py
def _sync_iter(
self, func: Callable[P, AsyncIterator[T]], *args: P.args, **kwargs: P.kwargs
) -> List[T]:
def _sync_iter(self, coroutine: Coroutine[Any, Any, AsyncIterator[T]]) -> List[T]:

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I removed the args/kwargs here to make it handle a coroutine, which is how it's so far being used.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @DahnJ. I've launched the workflows.

@DahnJ

DahnJ commented Mar 5, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @joshmoore I tried to fix a failing test (which I think happened because Any was used in a Pydantic model), can you launch them again please?

@joshmoore

Copy link
Copy Markdown
Member

On their way! 🏇🏽

@jhammanjhamman left a comment

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.

Thanks @DahnJ for jumping in here! Really happy to see this contribution.

Some guiding principles that may help the work here progress. First, I would prioritize getting Mypy to pass, even if it requires ignoring a few problematic lines. Then, once things are passing again (see #1649 where we disable the mypy checks in the v3 CI), then we can come back and fix any TODOs.

Comment threadsrc/zarr/v3/group.py Outdated
else:
return {
ZGROUP_JSON: self.zarr_format,
ZGROUP_JSON: str(self.zarr_format).encode(),

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.

Flagging that this was actually a bug in hiding. I believe it should have been something like:

Suggested change
ZGROUP_JSON: str(self.zarr_format).encode(),
ZGROUP_JSON: json.dumps({"zarr_format": 2}).encode(),

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Addressed in a30af88

Comment threadsrc/zarr/v3/store/core.py Outdated
@classmethod
def from_path(cls, pth: Path) -> StorePath:
return cls(Store.from_path(pth))
# NOT SOLVED: This is instantiating an ABC + there is no from_path method

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.

Good catch. I don't think we're using StorePath.from_path() anymore. If so, I'd be comfortable removing it here.

Comment threadsrc/zarr/v3/store/core.py Outdated
from upath import UPath

return StorePath(Store.from_path(UPath(store_like)))
# NOT SOLVED: Similar here, ABC instantiation + no from_path method

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.

See above.

@DahnJ

Copy link
Copy Markdown
ContributorAuthor

@jhamman I have addressed the comments in a30af88

I was not sure how to handle the secondStore.from_path() reference. What should happen if the store_like argument is of type str? I tried to make a potential suggestion based on a commented-out code in f57527a. Let me please know if this is the correct way of handling that case.

As for getting mypy to pass, it does pass for me locally and I didn't add any type: ignore lines. However, there already are a few in zarr/v3 – I removed some more in e1fdd1e and then turned on mypy in pre-commit in db6da34.

@jhamman

Copy link
Copy Markdown
Member

@DahnJ - I think this is a great step forward. I want @d-v-b to review this but from my perspective, it could go in now, even with the outstanding store issue.

@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

This looks good! There's some overlap with stuff I did over in #1743, but that's not a problem. I will merge and rebase #1743 on this.

@d-v-b
d-v-b merged commit 15a9747 into zarr-developers:v3Apr 6, 2024
@d-v-b

d-v-b commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

thanks @DahnJ !

d-v-b pushed a commit to d-v-b/zarr-python that referenced this pull request Apr 10, 2024
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
@d-v-bd-v-b mentioned this pull request Apr 12, 2024
6 tasks
d-v-b added a commit that referenced this pull request Apr 22, 2024
* chore: add deprecation warnings to v3 classes / functions
* Resolve Mypy erorrs in `v3` branch (#1692)
* refactor(v3): Using appropriate types
* fix(v3): Typing fixes + minor code fixes
* fix(v3): _sync_iter works with coroutines
* docs(v3/store/core.py): clearer comment
* fix(metadata.py): Use Any outside TYPE_CHECKING for Pydantic
* fix(zarr/v3): correct zarr format + remove unused method
* fix(v3/store/core.py): Potential suggestion on handling str store_like
* refactor(zarr/v3): Add more typing
* ci(.pre-commit-config.yaml): zarr v3 mypy checks turned on in pre-commit
* Specify hatch envs using GitHub actions matrix for v3 tests (#1728)
* Specify v3 hatch envs using GitHub actions matrix
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* Update .github/workflows/test-v3.yml
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
* test on 3.12 too
* no 3.12
---------
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
* black -> ruff format + cleanup (#1639)
* black -> ruff + cleanup
* format
* Preserve git blame
* pre-commit fix
* Remove outdated dev install docs from installation.rst and link to contributing.rst (#1643)
Co-authored-by: Joe Hamman <joe@earthmover.io>
* chore: remove old v3 implementation
* chore: remove more version-conditional logic
* chore: remove v3_storage_transformers.py again
---------
Co-authored-by: Daniel Jahn (dahn) <dahnjahn@gmail.com>
Co-authored-by: Max Jones <14077947+maxrjones@users.noreply.github.com>
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
Co-authored-by: Alden Keefe Sampson <aldenkeefesampson@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants

@DahnJ@joshmoore@jhamman@d-v-b@sanketverma1704