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

Reduce mypy errors (use sys.info instead of try-fail) #1969

Closed
wants to merge 1 commit into from
Closed

Reduce mypy errors (use sys.info instead of try-fail) #1969

wants to merge 1 commit into from

Conversation

MartinThoma
Copy link

No description provided.

@alangenfeld
Copy link
Member

Nice, thanks! Running our full CI then will merge

@MartinThoma
Copy link
Author

I'll have a look at that tomorrow:


************* Module dagster.seven
--
  | python_modules/dagster/dagster/seven/__init__.py:14:4: E0611: No name 'tempfile' in module 'backports' (no-name-in-module)
  | python_modules/dagster/dagster/seven/__init__.py:14:4: E0401: Unable to import 'backports.tempfile' (import-error)
  | python_modules/dagster/dagster/seven/__init__.py:30:4: E0401: Unable to import 'functools32' (import-error)

(Other topic: I'm a bit confused by seeing "buildkite/dagster/black — Passed (49 seconds)" - do you use the black formatter? Why does that pass? I'm pretty sure I have seen non-black formatted code)

@alangenfeld
Copy link
Member

do you use the black formatter? Why does that pass?

Ya you can check the Makefile in the root to see what args we use

@MartinThoma
Copy link
Author

When I run make check_black I get 53 files would be reformatted, 740 files would be left unchanged, 7 files would fail to reformat.

Picking just one of many examples: https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster/seven/abc.py#L45-L62 is not black formatting because of the ' - black would use ". Hence my confusion.

@alangenfeld
Copy link
Member

alangenfeld commented Dec 11, 2019

huh - which version of black are you running? We pin to 19.10b0 via python_modules/dagster/dev-requirements.txt and use -S / --skip-string-normalization which avoids coercing quotes.

@MartinThoma
Copy link
Author

Good catch! Yes, my black version was a bit outdated. I didn't have the -S flag.

@MartinThoma
Copy link
Author

Going back to the bug: I'm still a bit puzzled by it. If I read https://buildkite.com/dagster/dagster/builds/8064#f1449ea8-72ea-4350-a1b6-19b68d7a7726 correctly, then it runs from a docker image 968703565975.dkr.ecr.us-west-1.amazonaws.com/buildkite-integration:py3.7.4-v6 - the name indicates that Python 3.7 is executed there. But the failure (__init__.py:14:4: E0611: No name 'tempfile' in module 'backports') indicates that sys.version_info[0] <= 2 is true.

@MartinThoma
Copy link
Author

I understand the problem now, but it might be that it is not possible to make pylint and mypy happy at the moment. python/mypy#1393 (comment) could be a blocker. Let's see, maybe somebody knows a work-around: https://stackoverflow.com/q/59295045/562769

@alangenfeld
Copy link
Member

not possible to make pylint and mypy happy

I think we would be fine with suppression comments to disable pylint assuming it's just in a few spots.

@mgasner
Copy link
Contributor

mgasner commented Dec 14, 2019

apologies, these test failures on windows are my fault -- if you rebase on master these should go away

@mgasner
Copy link
Contributor

mgasner commented Dec 19, 2019

Windows failures are resolved on master so I think this is gtg.

@mgasner mgasner self-requested a review December 19, 2019 17:27
@mgasner
Copy link
Contributor

mgasner commented Jan 7, 2020

Let's rebase on master to kick the CI off again and then should be good to go when all checks are green.

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.

3 participants