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.

Sign up for GitHub

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

Jump to bottom

Make psutil an optional dependency #4634

Merged
merged 8 commits into from
Feb 26, 2018
Merged

Make psutil an optional dependency #4634

merged 8 commits into from
Feb 26, 2018

Conversation

miedzinski
Copy link
Contributor

@miedzinski miedzinski commented Feb 25, 2018 •
edited
Loading

  • in case of psutil not installed, daemon reports it and suggests installing it
  • on Windows it isn't declared as dependency nor reported as missing

Fixes #4593.

@miedzinski
Copy link
Contributor Author

Perhaps a better extra name could be used? I've picked dmypy, because psutil is used there.

emmatyping
emmatyping requested changes Feb 25, 2018
View reviewed changes
Copy link
Collaborator

@emmatyping emmatyping left a comment

Choose a reason for hiding this comment

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

Looks like a good start, but this should be documented (maybe in the installation section?). Also, I think there should be an error message here

mypy/mypy/dmypy_server.py

Line 363 in 0c12b21

try:
if it tries to import psutil and fails. Maybe psutil not found, run pip install mypy[dmypy] to install the needed components for dmypy?

setup.py Outdated
],
extras_require = {
':python_version < "3.5"': 'typing >= 3.5.3',
'dmypy': 'psutil >= 5.4.0, < 5.5.0',
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should add :sys_platform!="win32" after dmypy, since it is not used on Windows.

@miedzinski
Copy link
Contributor Author

miedzinski commented Feb 25, 2018 •
edited
Loading

I agree that docs should be updated, but I don't know what dmypy actually is. All I could do is to mention an extra dependency, but without any explanation about its purpose. And dmypy isn't documented at all anyway.

@miedzinski
Copy link
Contributor Author

Oh and I'm pretty sure that using print there is wrong, because I don't see any output when running dmypy. Is there any logger I can't find?

@gvanrossum
Copy link
Member

Can you point me to the docs that explain this notation works? Both the 'dmypy': prefix in the extras_require list and the pip install mypy[dmypy] form are new to me.

I think the best way to surface the error is to stick it in the res dictionary, e.g. with the key 'memory_psutil_missing'.

@miedzinski
Copy link
Contributor Author

@gvanrossum
https://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-extras-optional-features-with-their-own-dependencies
https://packaging.python.org/tutorials/installing-packages/#installing-setuptools-extras
https://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-platform-specific-dependencies

@gvanrossum
Copy link
Member

Assuming the tests pass I'm fine with it.

@emmatyping
Copy link
Collaborator

I restarted the 3.6 build on Travis as the stubgen tests seem to be flaking.

@miedzinski
Copy link
Contributor Author

I've updated PR description in such way that it describes all changes to the codebase.

JukkaL
JukkaL requested changes Feb 26, 2018
View reviewed changes
docs/source/getting_started.rst Outdated
@@ -13,6 +13,12 @@ you can install mypy with:

$ python3 -m pip install mypy

Optionally you can install extra dependencies for daemon server with:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this since dmypy is otherwise undocumented (and unsupported). This will only confuse users.

@gvanrossum gvanrossum merged commit 55dadfc into python:master Feb 26, 2018
@gvanrossum
Copy link
Member

Thanks!

carljm added a commit to carljm/mypy that referenced this pull request Feb 28, 2018
@carljm
Merge branch 'master' into import-cycle
2a37e80 
* master: (27 commits)
  Don't call --strict-optional and --incremental experimental (python#4642)
  Sync typeshed (python#4641)
  Fix callable types with inconsistent argument counts (python#4611)
  Fix example (add 'class A:')
  Make psutil an optional dependency (python#4634)
  mypy and mypy_extensions aren't posix only (python#3765)
  Documentation for attr support (python#4632)
  Use read_with_python_encoding in stubgen to handle file encoding (python#3790)
  Sync typeshed (python#4631)
  Add remaining core team emails to CREDITS (python#4629)
  Fix issues with attr code. (python#4628)
  Better support for converter in attrs plugin. (python#4607)
  Clean up credits (python#4626)
  Support type aliases in fine-grained incremental mode (python#4525)
  Fine-grained: Fix crash caused by unreachable class (python#4613)
  Treat divmod like a binary operator (python#4585)
  Sync typeshed (python#4605)
  Fine-grained: Don't infer partial types from multiple targets (python#4553)
  Fine-grained: Compare symbol table snapshots when following dependencies (python#4598)
  Fix type of forward reference to a decorated class method (python#4486)
  ...
yedpodtrzitko pushed a commit to kiwicom/mypy that referenced this pull request Mar 15, 2018
@miedzinski @yedpodtrzitko
Make psutil an optional dependency (python#4634)
fa599b0 
If it's not installed, when running `dmypy status -v` a message will be displayed instead of the memory stats.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JukkaL JukkaL JukkaL approved these changes

@emmatyping emmatyping emmatyping approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@miedzinski @gvanrossum @emmatyping @JukkaL