-
Notifications
You must be signed in to change notification settings - Fork 613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use hatch instead of poetry #445
base: main
Are you sure you want to change the base?
Conversation
feff38b
to
bf5d8e9
Compare
@@ -97,7 +98,7 @@ def get(self, key: str, default: Any = None) -> Any: | |||
>>> msg.get('tool_calls')[0]['function']['name'] | |||
'foo' | |||
""" | |||
return self[key] if key in self else default | |||
return getattr(self, key) if hasattr(self, key) else default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
linter suggests self.get(k, default)
but that'll call itself
@@ -15,12 +15,12 @@ def _parse_docstring(doc_string: Union[str, None]) -> dict[str, str]: | |||
if not doc_string: | |||
return parsed_docstring | |||
|
|||
key = hash(doc_string) | |||
key = str(hash(doc_string)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy
complains about a type mismatch for key
since hash(...)
returns an int
but may be set to a 'args'
, i.e. string, later
@@ -991,7 +993,7 @@ async def create( | |||
parameters: Optional[Union[Mapping[str, Any], Options]] = None, | |||
messages: Optional[Sequence[Union[Mapping[str, Any], Message]]] = None, | |||
*, | |||
stream: Literal[True] = True, | |||
stream: Literal[False] = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This overload was incorrect since stream=True is also overloaded below
@@ -1054,19 +1056,19 @@ async def create( | |||
|
|||
async def create_blob(self, path: Union[str, Path]) -> str: | |||
sha256sum = sha256() | |||
with open(path, 'rb') as r: | |||
async with await anyio.open_file(path, 'rb') as r: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not use a blocking open in an async function
- run: pipx install poetry | ||
- uses: actions/setup-python@v5 | ||
with: | ||
python-version: ${{ matrix.python-version }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for removing the matrix tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is replaced by hatch test --all
which tests all configured python versions. I'm still debating whether this is better than matrix tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we can setup a matrix if needed in the pyproject.toml
if needed: https://hatch.pypa.io/latest/tutorials/testing/overview/#all-environments
I think it makes sense to just define the versions we want tested in the toml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's already implemented and runs with --all
option to hatch test
however it is slightly slower since the Python versions are sequential rather than parallel, ~1m vs. ~35s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would parallelizing the tests themselves help? https://hatch.pypa.io/1.13/tutorials/testing/overview/#parallelize-test-execution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that option, which is also set, parallelizes tests in a pytest run, not between python versions
this change decouples project management from virtualenv management such that developers can use their favorite tools instead of being forced into poetry.
poetry 2.0 adds pep 621 support so updating pyproject.toml to be pep 621 compliant requires many of the changes necessary to use hatch while also having to manually maintain separate dependency groups for dev tools such as pytest and ruff
fix some linting issues found by newly enabled linters
'://'.join(scheme, hostport)
withf'{scheme}://{hostport}'
pass
startswidth
withstartswith(tuple)
contextlib.suppress
instead oftry
except
pass
isinstance
withisinstance(tuple)
if
else