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] extend main ruff config + move boto3-stubs to stubs extras #24153

Conversation

danielgafni
Copy link
Contributor

@danielgafni danielgafni commented Sep 2, 2024

Summary & Motivation

  1. We currently have boto3-stubs-lite in dagster-aws runtime dependencies. This might be causing type-checkers to fail for our users if they have "bad" boto3 code (which previously wasn't checked because no stubs package was installed). This PR moves boto3-stubs-lite to a new stubs extras of dagster-aws, ensuring it won't be installed by default. It's now only imported under if TYPE_CHECKING blocks. Packages like mypy_boto3_glue were added to banned top-level modules (via ruff rule).

  2. Additionally, dagster-aws's ruff config now extends the root config. This mostly fixed import ordering, previously dagster_aws imports weren't coming after all other packages.

How I Tested These Changes

Changelog [New | Bug | Docs]

NOCHANGELOG

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @danielgafni and the rest of your teammates on Graphite Graphite

@danielgafni danielgafni marked this pull request as ready for review September 2, 2024 16:11
@graphite-app graphite-app bot added the area: docs Related to documentation in general label Sep 2, 2024
@danielgafni danielgafni requested review from alangenfeld and schrockn and removed request for erinkcochran87 September 2, 2024 16:13
@danielgafni danielgafni force-pushed the 09-02-_dagster-aws_extend_main_ruff_config_move_boto3-stubs_to_pyright_extras branch 3 times, most recently from 4717282 to 6df7cb0 Compare September 2, 2024 16:43
@danielgafni danielgafni removed the area: docs Related to documentation in general label Sep 2, 2024
@danielgafni
Copy link
Contributor Author

danielgafni commented Sep 2, 2024

pyright failing because of broken master (will try to rebase, seems to be fixed now)

@danielgafni danielgafni force-pushed the 09-02-_dagster-aws_extend_main_ruff_config_move_boto3-stubs_to_pyright_extras branch from 6df7cb0 to 07f3e28 Compare September 2, 2024 19:21
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.

Hmm this seems complicated.

The errors that users would see are not phantom errors, correct? They would be real issues?

I don't think this is an unreasonable change, and users can easily ignore it with type: ignore.

Is this in response to user feedback?

@danielgafni
Copy link
Contributor Author

danielgafni commented Sep 3, 2024

The errors that users would see are not phantom errors, correct? They would be real issues?

Well, kinda, but not exactly. But there are 2 arguments against them:

  • This will break existing builds for dagster-aws users once they update to a newer version. The errors will be popping out in random places, anywhere where boto3 is used, even outside of Dagster code. It might be confusing for users unaware of boto3-stubs.
  • The number of errors might be overwhelming. boto3-stubs is really strict (and probably that's technically correct and aligned to AWS API specs). For example, most of response fields are set to NotRequired, so the user must do response.get("field") instead of response["field"]. And this might be a problem, because pyright doesn't have a way to automatically insert # type: ignore comments (if you remember, I made a script for that). Because vanilla boto3 is basically untyped, old codebases will be drowning in sudden type-checking errors.

In conclusion, this doesn't seem right to me, it should be a conscious decision to enable type-checking for boto3 on the user side. We shouldn't be forcing best-practices onto users.

Is this in response to user feedback?

No, but I am confident the users are going to complain about it

@schrockn
Copy link
Member

schrockn commented Sep 3, 2024

Ah it will be break all all boto3 API calls. Yes then I agree this is too aggressive.

I just don't know if this is worth all the trouble and complexity at this point. How many people are going to go through the trouble of installing the appropriate extra? Let's discuss in our meeting today.

@schrockn
Copy link
Member

schrockn commented Sep 3, 2024

Per offline discussion let's name the extra [stubs] and this is useful for our own internal development. Let's rename it and move forward.

@danielgafni danielgafni force-pushed the 09-02-_dagster-aws_extend_main_ruff_config_move_boto3-stubs_to_pyright_extras branch from 07f3e28 to 8060825 Compare September 9, 2024 09:13
@danielgafni danielgafni changed the title [dagster-aws] extend main ruff config + move boto3-stubs to "pyright" extras [dagster-aws] extend main ruff config + move boto3-stubs to stubs extras Sep 9, 2024
@danielgafni danielgafni force-pushed the 09-02-_dagster-aws_extend_main_ruff_config_move_boto3-stubs_to_pyright_extras branch from 8060825 to f7503e3 Compare September 9, 2024 09:20
@danielgafni danielgafni requested a review from schrockn September 9, 2024 09:37
@danielgafni
Copy link
Contributor Author

@schrockn done

@danielgafni danielgafni force-pushed the 09-02-_dagster-aws_extend_main_ruff_config_move_boto3-stubs_to_pyright_extras branch from f7503e3 to bd00566 Compare September 12, 2024 14:22
@danielgafni danielgafni force-pushed the 09-02-_dagster-aws_extend_main_ruff_config_move_boto3-stubs_to_pyright_extras branch from bd00566 to e69f610 Compare September 12, 2024 14:23
@danielgafni danielgafni merged commit ea5ca86 into master Sep 12, 2024
1 of 2 checks passed
@danielgafni danielgafni deleted the 09-02-_dagster-aws_extend_main_ruff_config_move_boto3-stubs_to_pyright_extras branch September 12, 2024 14:53
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.

None yet

2 participants