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

feat: kudos, users and more stats endpoints; docs/style: use_attribute_docstrings, apply D ruff rule set, better examples #198

Merged
merged 25 commits into from
May 30, 2024

Conversation

tazlin
Copy link
Member

@tazlin tazlin commented May 23, 2024

  • /v2/kudos, /v2/users and /v2/stats endpoints (and their variations) are now supported
  • use_attribute_docstrings allows docstrings to be converted to the description field of pydantic BaseModels, which in turn allows for that information to be propagated to the documentation
  • Applies pydocstyle (ruff rule D), which includes adding a large number of missing docstrings and fixing their tone, missing args and styling.
  • Provides more and better quality examples. See the examples folder for more information.
  • chore: fix codegen readme; ignore gen'd file
  • fix: don't use open in async example

tazlin added 23 commits May 23, 2024 18:15
- `use_attribute_docstrings` allows docstrings to be converted to the `description` field of pydantic `BaseModel`s, which in turn allows for that information to be propagated to the documentation
- Applies `pydocstyle` (ruff rule `D`), which includes adding a large number of missing docstrings and fixing their tone, missing args and styling.
- chore: fix codegen readme; ignore gen'd file
- fix: don't use `open` in async example
- Removes no-longer-present args from docstrings
- Formats docstrings so `griffe` (a `mkdocs` dep) can parse them
Due to the fact that hashing was being done on based on job ID, bugs would arise because `__eq__`/`__hash__` checks caused different object types to return matches between each other (e.g., `ImageGenerateStatusResponse` and `ImageGenerateAsyncResponse` matching each other on an `in` check). The revised implementation now discriminates identity based on these magic functions by also including the class name (`cls.__name__`) in the hash calculation as a fixed offset.
This reflects a philosophical shift on my part. I previously would use a `with` statement with the specified horde client but I have come to the conclusion that it should be the caller's responsibility to do this at a time of their choosing. The status quo before this change would/could cause situations where requests would be cancelled in an almost certainly unintended manner. For example, if you had two concurrent simple requests going, the requests would race to finish and the first completing would cause the second to be cancelled.
The API treats false-y values for `type` on filter `GET` requests as "no filter".
Also fixes a naming inconsistency with (now) `ImageStatsModelsResponse`
also changes the undocumented/anonymous model identifier to the const `_ANONYMOUS_MODEL `
@tazlin tazlin changed the title docs/style: use_attribute_docstrings, apply D ruff rule set feat: kudos, users and more stats endpoints; docs/style: use_attribute_docstrings, apply D ruff rule set, better examples May 30, 2024
@tazlin tazlin marked this pull request as ready for review May 30, 2024 14:27
@tazlin tazlin merged commit 64af40a into main May 30, 2024
4 checks passed
@tazlin
Copy link
Member Author

tazlin commented May 31, 2024

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

5, because the PR is extensive with multiple files and significant changes across various modules, including new features, refactoring, and documentation updates. The complexity and breadth of the changes require a thorough review to ensure functionality, performance, and security are maintained or enhanced.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The use of PROGRESS_STATE enum in consts.py might not be fully integrated into the existing system if the rest of the codebase does not handle these new states.

Performance Concern: The addition of multiple new features and models might impact the performance if not properly optimized, especially in handling large data or high traffic.

🔒 Security concerns

No

Code feedback:
relevant filehorde_sdk/ai_horde_api/apimodels/workers/_workers.py
suggestion      

Consider adding validation for the maintenance_msg field in ModifyWorkerRequest to ensure it does not exceed a reasonable length and does not contain any inappropriate content. [important]

relevant linemaintenance_msg: str | None = Field(None)

relevant filehorde_sdk/ai_horde_api/apimodels/generate/_pop.py
suggestion      

It's recommended to implement a method to handle the scenario when bridge_version is not provided in ImageGenerateJobPopRequest, perhaps by setting a sensible default or rejecting requests that do not meet the minimum required version. [important]

relevant linebridge_version: int | None = None

relevant filehorde_sdk/ai_horde_api/apimodels/base.py
suggestion      

For the JobRequestMixin and JobResponseMixin classes, consider implementing a more robust ID validation system that not only checks for non-empty values but also ensures the format of the ID meets certain criteria (e.g., length, character set). [important]

relevant line"""Ensure that the job ID is not empty."""

relevant filehorde_sdk/ai_horde_api/apimodels/_status.py
suggestion      

In HordePerformanceResponse, consider adding a method to calculate the average performance based on the thread_count and worker_count to provide more actionable insights into the system's performance. [medium]

relevant line"""How many worker threads are actively processing prompt generations in this {horde_noun} in the past 5

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