Skip to content

fix: Address mypy typing errors in v2 SDK#157

Merged
awalker4 merged 8 commits intomainfrom
fix/typing-errors-v2
Aug 21, 2024
Merged

fix: Address mypy typing errors in v2 SDK#157
awalker4 merged 8 commits intomainfrom
fix/typing-errors-v2

Conversation

@awalker4
Copy link
Copy Markdown
Collaborator

@awalker4 awalker4 commented Aug 20, 2024

Fix all mypy errors due to incorrect typing after the SDK v2 merge (#135). Logic changes should be minimal, this is mostly to change type hints where a httpx.Response is used instead of a requests.Response, etc. I removed some form_utils.py functions where we no longer need to convert a httpx request back to Requests. There's more we can cleanup in here, but let's get the V2 migration settled first.

Add mypy to make lint so that we can cath these errors before merging. The publish job runs a full linter suite, and these changes made it to main but broke the publish job.

Also, remove the Patch Custom Code step that I added to the generate. This broke the job. There are some minor changes to the Speakeasy code on the main branch. In the short term, this means we'll have to run make patch-custom-code whenever we regenerate.

To verify

Make sure you can lint and run the tests locally. make lint and make test. You can also verify that the pdf split behavior has not changed with a call to your local server:

from unstructured_client import UnstructuredClient
from unstructured_client.models import shared, operations

import json

filename = "_sample_docs/layout-parser-paper.pdf"

s = UnstructuredClient(
    server_url="http://localhost:8000",
)

with open(filename, "rb") as f:
    files=shared.Files(
        content=f,
        file_name=filename,
    )

    req = operations.PartitionRequest(
        shared.PartitionParameters(
            files=files,
            strategy="fast",
            split_pdf_page_range=[4,8],
        ),
    )

    resp = s.general.partition(req)
    print(json.dumps(resp.elements, indent=4))

Update all typing that used requests to take httpx classes instead. Logic changes should be minimal,
this is mostly to change type hints where a `httpx.Response` is used instead of a
`requests.Response`, etc.

Add mypy to `make lint` so that we can cath these errors before merging. The publish job runs a full
linter suite, and these changes made it to main but broke the publish job.
@awalker4 awalker4 changed the title fix: Address mypy typing errors in v2 SDK fix: Address mypy typing errors in v2 SDK (DRAFT) Aug 20, 2024
@awalker4 awalker4 force-pushed the fix/typing-errors-v2 branch from 1de02e3 to c9f00ce Compare August 20, 2024 23:26
@awalker4 awalker4 changed the title fix: Address mypy typing errors in v2 SDK (DRAFT) fix: Address mypy typing errors in v2 SDK Aug 20, 2024
Copy link
Copy Markdown
Contributor

@pawel-kmiecik pawel-kmiecik left a comment

Choose a reason for hiding this comment

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

Found a nit, but generally LGTM

Copy link
Copy Markdown
Contributor

@pawel-kmiecik pawel-kmiecik left a comment

Choose a reason for hiding this comment

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

I've just noticed that with V2 poetry is required.
Previously it was just set as a build tool but everything worked with pip.
Now, the dependencies are defined as poetry deps in pyproject.toml so they are not installed with the package (like pydantic) if installing with pip install -e ..

I think it requires adding some Makefile commands to install the package properly (with poetry install) or adding install steps in the README

@pawel-kmiecik pawel-kmiecik dismissed their stale review August 21, 2024 15:03

Don't want to block the PR due to timezone gap

@awalker4
Copy link
Copy Markdown
Collaborator Author

I've just noticed that with V2 poetry is required. Previously it was just set as a build tool but everything worked with pip. Now, the dependencies are defined as poetry deps in pyproject.toml so they are not installed with the package (like pydantic) if installing with pip install -e ..

I think it requires adding some Makefile commands to install the package properly (with poetry install) or adding install steps in the README

Ah, good call! I'm not very familiar with poetry but I suppose this is the time to switch over the makefile commands where it's needed. I'll throw in a new issue for this, as right now I'm trying to unblock the split page retry fixes, and CI does seem to be installing everything as needed.

@awalker4 awalker4 requested a review from badGarnet August 21, 2024 17:55
* Fix some verbose isinstance checks
* Use single line for install-dev
@awalker4 awalker4 force-pushed the fix/typing-errors-v2 branch from fd93aa4 to 39f182d Compare August 21, 2024 17:56
Copy link
Copy Markdown
Contributor

@Coniferish Coniferish left a comment

Choose a reason for hiding this comment

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

LGTM

@awalker4 awalker4 merged commit 2f3c098 into main Aug 21, 2024
@awalker4 awalker4 deleted the fix/typing-errors-v2 branch August 21, 2024 21:34
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.

4 participants