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

Add py.typed file to package #346

Merged
merged 1 commit into from
Jan 31, 2025
Merged

Conversation

thomashopkins32
Copy link
Contributor

Description

Add py.typed marker for the package.

Motivation and Context

When using ophyd-async, the mypy extension in VS Code complains that there is no py.typed stub in event-model.

How Has This Been Tested?

@evalott100
Copy link
Contributor

This breaks bluesky's tox -e type-checking. There should probably be a PR there to fix?

@thomashopkins32
Copy link
Contributor Author

@evalott100 Ah I'm guessing because bluesky has typing errors related to event-model objects?

@evalott100
Copy link
Contributor

evalott100 commented Jan 16, 2025

Ah I'm guessing because bluesky has typing errors related to event-model objects?

It's because we have some TypedDict e.g datum_doc which we know is Datum, but type checking sees as some union of documents because it comes from a structure typed as such.

https://github.com/bluesky/bluesky/blob/ca4f8aa136c91dccd428936d2d0ebe963b925f4e/src/bluesky/callbacks/tiled_writer.py#L197-L202

Here _docs_cache values are typed as Datum | Resource | StreamResource so you get key errors on "datum_id" for those possibilities. I would see if you can clean up the typing, though I think most of these need to be handled with casting, i.e

                    datum_doc = typing.cast(Datum, self._docs_cache.pop(datum_id))

since there's no way the type checker will know that an element with uid datum_id has to be a Datum.

I think this is the format of most of the issues with type checking on bluesky for this PR.

@thomashopkins32
Copy link
Contributor Author

Okay, I'll try to take a stab at it and see if I can get it to work. Thanks for the info!

@evalott100
Copy link
Contributor

evalott100 commented Jan 16, 2025

Okay, I'll try to take a stab at it and see if I can get it to work. Thanks for the info!

Cheers!

Instead of casting, the above example can probably be solved by making individual _datum_docs_cache, _stream_resource_docs_cache and _resource_docs_cache, then adding to the respective cache in these:

https://github.com/bluesky/bluesky/blob/ca4f8aa136c91dccd428936d2d0ebe963b925f4e/src/bluesky/callbacks/tiled_writer.py#L212-L219

This would be nicer, and actually lead to better code across the file imo.

For some places you might have to cast instead though. If there's somewhere where a nicer typing solution is feasible but you don't have the time then just adding casts for now is probably fine.

@thomashopkins32
Copy link
Contributor Author

@evalott100 I finished the changes to bluesky that are blocking this PR from being merged. Please see bluesky/bluesky#1873

I tried my best not to simply cast where I could but indeed some places it was necessary.

Copy link
Contributor

@evalott100 evalott100 left a comment

Choose a reason for hiding this comment

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

Bluesky changes to accomodate this ready to be merged as well.

@thomashopkins32 thomashopkins32 merged commit 7cb9543 into bluesky:main Jan 31, 2025
12 checks passed
@thomashopkins32 thomashopkins32 deleted the py-typed branch January 31, 2025 15:23
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