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

[dagster-aws] ignore pyright errors with boto3-stubs #23954

Conversation

danielgafni
Copy link
Contributor

@danielgafni danielgafni commented Aug 27, 2024

Summary & Motivation

Ignore pyright errors in dagster-aws which popped out after the introduction of boto3-stubs-lite.

I suggest we slowly remove these ignore comments and fix the code for pyright to pass over time.

I made a script to do it automatically.

How I Tested These Changes

pyright should be passing now

Changelog [New | Bug | Docs]

NOCHANGELOG

Copy link
Contributor Author

danielgafni commented Aug 27, 2024

@danielgafni danielgafni marked this pull request as ready for review August 27, 2024 10:35
@danielgafni danielgafni force-pushed the 08-27-_dagster-aws_ignore_pyright_errors_with_boto3-stubs branch 2 times, most recently from 3e312a4 to cba2bfc Compare August 27, 2024 11:16
@danielgafni
Copy link
Contributor Author

pyright is currently only failing because of broken build in master

@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch from 7f389db to eac4e63 Compare August 28, 2024 07:52
@danielgafni danielgafni force-pushed the 08-27-_dagster-aws_ignore_pyright_errors_with_boto3-stubs branch 3 times, most recently from ebf9569 to 469c63a Compare August 28, 2024 09:42
@danielgafni danielgafni requested a review from schrockn August 28, 2024 09:42
@schrockn
Copy link
Member

Can you give some more context on these type errors, why the code is working now, and what the likely fix is? Is there a pattern here?

@danielgafni
Copy link
Contributor Author

Most of them are because of accessing keys in boto3 response dicts.
For some reason most of them are flagged as NonRequired, so accessing with [] instead of .get() raises a pyright error now.
I think it's fine to ignore these errors since this code actually never raised runtime errors (so in practice these keys are always set).

@schrockn
Copy link
Member

schrockn commented Aug 28, 2024 via email

Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

Script is pretty great

@danielgafni danielgafni force-pushed the 08-09-_dagster-aws_add_ecs_pipes branch from eac4e63 to ffc36c4 Compare August 28, 2024 15:52
@danielgafni danielgafni force-pushed the 08-27-_dagster-aws_ignore_pyright_errors_with_boto3-stubs branch from 469c63a to 5ce998f Compare August 28, 2024 15:52
@danielgafni
Copy link
Contributor Author

I wonder why pyright doesn't have an option to autoignore errors (ruff does)

@danielgafni danielgafni merged commit a0f68ca into 08-09-_dagster-aws_add_ecs_pipes Aug 28, 2024
1 check was pending
@danielgafni danielgafni deleted the 08-27-_dagster-aws_ignore_pyright_errors_with_boto3-stubs branch August 28, 2024 16:06
danielgafni added a commit that referenced this pull request Aug 28, 2024
## Summary & Motivation

Ignore pyright errors in `dagster-aws` which popped out after the
introduction of `boto3-stubs-lite`.

I suggest we slowly remove these ignore comments and fix the code for
pyright to pass over time.

I made a script to do it automatically. 

## How I Tested These Changes

pyright should be passing now

## Changelog [New | Bug | Docs]

> NOCHANGELOG
danielgafni added a commit that referenced this pull request Aug 28, 2024
## Summary & Motivation

Ignore pyright errors in `dagster-aws` which popped out after the
introduction of `boto3-stubs-lite`.

I suggest we slowly remove these ignore comments and fix the code for
pyright to pass over time.

I made a script to do it automatically. 

## How I Tested These Changes

pyright should be passing now

## Changelog [New | Bug | Docs]

> NOCHANGELOG
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