Closed landgraf closed 1 month ago
if getpass is not installed
@landgraf getpass
is a standard library from Python. I wonder why it will raise an ImportError
. In general, please file an issue first before submitting a PR.
Also, the exception that raised by getpass
will be changed in the future.
if getpass is not installed
@landgraf
getpass
is a standard library from Python. I wonder why it will raise anImportError
. In general, please file an issue first before submitting a PR.Also, the exception that raised by
getpass
will be changed in the future.
I was wrong, it's ModuleNotFoundError exception which is being raised [1] so the patch is even smaller and makes more sense now. Updated the patch.
In yocto project based linux distribution getpass is provided by python3-unixadmin package which should be installed explicitly and I don't want to bring additional dependency just because of getpass. There's only one single usage of getpass in the nvitop which is covered by try-except anyway.
[1]
$ nvitop
Traceback (most recent call last):
File "/usr/bin/nvitop", line 5, in <module>
from nvitop.cli import main
File "/usr/lib/python3.12/site-packages/nvitop/init.py", line 24, in <module>
from nvitop.select import select_devices
File "/usr/lib/python3.12/site-packages/nvitop/select.py", line 60, in <module>
import getpass
ModuleNotFoundError: No module named 'getpass'
In yocto project based linux distribution getpass is provided by python3-unixadmin package which should be installed explicitly
@landgraf I think you should file an issue there. Sorry.
and I don't want to bring additional dependency just because of getpass.
You can do this:
SITE_PACKAGES="$(python3 -c "import sysconfig; print(sysconfig.get_path('purelib'))")"
echo "def getuser(): raise ModuleNotFoundError" > "${SITE_PACKAGES}/getpass.py"
It will create a fake getpass
module.
@landgraf I think you should file an issue there. Sorry.
No, @landgraf should not. It's not the Yocto issue. On some systems Python might be redistributed without some standard library parts. Your solution is an ugly hack. The proposed patch only makes your software better adapted to such platforms. Rejecting patches that make your code better is not in the open-source spirit.
It's not the Yocto issue. On some systems Python might be redistributed without some standard library parts.
@landgraf @quarckster I don't really know the context of this issue and patch since we want to support a non-standard Python distribution. If you can open an issue and explain the rationale, I would be delighted to help.
The proposed patch only makes your software better adapted to such platforms. Rejecting patches that make your code better is not in the open-source spirit.
The patch as of now is not actually working. I think we will need a getuser()
function to work while under a container.
$ docker run -it --rm python:3-slim
Python 3.12.4 (main, Jul 2 2024, 20:57:30) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import getpass
>>> getpass.getuser()
'root'
>>> import os
>>> os.getlogin()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
OSError: [Errno 6] No such device or address
>>> exit()
We can discuss the solution further about either vendoring the getuser()
function or putting it in a try-except block with more edge case handling.
See also:
The patch as of now is not actually working.
The patch as of now does work. The issue you're hitting with container is different one and has nothing to do with the patch.
@XuehaiPan There're few issues with this part of the code:
1) getpass not always available. This PR solves this by importing it under try, this fix is harmless because it's the only usage of the getpass. I could work this around by installing python3-unixadmin package in the distribution but better to fix the issue properly.
2) failback to os.getlogin() not working in some envoriments (like containers and/or shellhub etc). It has nothing to do with (1) and was masked by it. You can try to add second failback to something like pwd.getpwuid(os.getuid())[0]
which works reliably in my environment but I can imagine other usecases where os.getlogin() is more preferable.
If neither of this works the code can exit gracefully with "Please install getpass, pwd, etc" error, not crash. As I have mentioned this is different issue and out of scope of this PR.
I could work this around by installing python3-unixadmin package in the distribution but better to fix the issue properly.
@landgraf I proposed a more robust version to resolve this. Let me know if it works for you and you are happy with it.
pip3 install git+https://github.com/XuehaiPan/nvitop.git@getuser
I could work this around by installing python3-unixadmin package in the distribution but better to fix the issue properly.
@landgraf I proposed a more robust version to resolve this. Let me know if it works for you and you are happy with it.
pip3 install git+https://github.com/XuehaiPan/nvitop.git@getuser
Your PR neither fixes the issue nor address previous comments here the only thing it does is introducing contextlib.suppress and changes the author. In other words it doesn't work and I'm not happy with it. Please take into account that not all distributions uses pip to install python libraries and as it was mentioned already python may be redistributed without standard library parts.
Your PR neither fixes the issue nor address previous comments here the only thing it does is introducing contextlib.suppress and changes the author. In other words it doesn't work and I'm not happy with it.
@landgraf The whole getuser()
function is under the suppress context. The getpass
module will be imported inside the function which is also under the suppress context. It should work as you expected.
Please take into account that not all distributions uses pip to install python libraries and as it was mentioned already python may be redistributed without standard library parts.
I mean you can try the change in the PR locally to see if it works.
the only thing it does is introducing contextlib.suppress
I am a bit confused that is contextlib
included in Yocto's Python?
this fixes crash on system without getpass installed
Issue Type
Runtime Environment
Yocto based linux os image
Description
Move import under try-except to not fail if getpass is not installed
Motivation and Context
Testing
ran nvitop with and without the changes
Images / Videos