Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

use hatch instead of poetry #445

wants to merge 1 commit into from

Conversation

mxyng
Copy link
Collaborator

@mxyng mxyng commented Feb 14, 2025

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

  • C416 - unnecessary list comprehension
  • FBT001 - positional boolean function arguments
  • FLY002 - replace '://'.join(scheme, hostport) with f'{scheme}://{hostport}'
  • PIE790 - unnecessary pass
  • PIE810 - replace multiple startswidth with startswith(tuple)
  • SIM105 - use contextlib.suppress instead of try except pass
  • SIM101 - replace multiple isinstance with isinstance(tuple)
  • SIM108 - use ternary operator instead of if else

@mxyng mxyng force-pushed the mxyng/hatch branch 2 times, most recently from feff38b to bf5d8e9 Compare February 14, 2025 09:20
@@ -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
Copy link
Collaborator Author

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))
Copy link
Collaborator Author

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,
Copy link
Collaborator Author

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:
Copy link
Collaborator Author

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 }}
Copy link
Contributor

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?

Copy link
Collaborator Author

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

Copy link
Contributor

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

Copy link
Collaborator Author

@mxyng mxyng Feb 20, 2025

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants