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

Improve get_ip_interface error message when interface does not exist #2964

Merged
merged 3 commits into from Aug 21, 2019

Conversation

@lesteve
Copy link
Contributor

commented Aug 19, 2019

Context: in dask-jobqueue you need some tweaking to find the right network interface to use on your cluster. People may not be familiar with what a network interface is and the current error message is a bit cryptic.

The main thing I am wondering about is whether we should keep the same error type KeyError with backward-compatibility in mind or switch to ValueError since ValueError is already the type of the error you get when the network interface does not have a valid IPv4 address.

My personal preference would be to switch to ValueError but I have kept KeyError for now.

Error message on master:

In [1]: from distributed.utils import get_ip_interface 
   ...: get_ip_interface('asdf')                                                                                                                                                   
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-1-cf8f3124444e> in <module>
      1 from distributed.utils import get_ip_interface
----> 2 get_ip_interface('asdf')

/home/local/lesteve/dev/distributed/distributed/utils.py in get_ip_interface(ifname)
    170     import psutil
    171 
--> 172     for info in psutil.net_if_addrs()[ifname]:
    173         if info.family == socket.AF_INET:
    174             return info.address

KeyError: 'asdf'

Error message in this PR:

In [1]: from distributed.utils import get_ip_interface 
   ...: get_ip_interface('asdf')                                                                                                                                                   
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-1-cf8f3124444e> in <module>
      1 from distributed.utils import get_ip_interface
----> 2 get_ip_interface('asdf')

/home/local/lesteve/dev/distributed/distributed/utils.py in get_ip_interface(ifname)
    175         allowed_ifnames = list(net_if_addrs.keys())
    176         raise KeyError('{!r} is not a valid network interface. '
--> 177                          'Valid network interfaces are: {}'.format(ifname, allowed_ifnames))
    178 
    179     for info in net_if_addrs[ifname]:

KeyError: "'asdf' is not a valid network interface. Valid network interfaces are: ['lo', 'enp0s25', 'br-e1ab2121f792', 'docker0', 'br-9f15079dee0d']"
@lesteve lesteve referenced this pull request Aug 19, 2019
expected_error_message += ".+'lo0'"
elif sys.platform.startswith("linux"):
expected_error_message += ".+'lo'"
with pytest.raises(KeyError, match=expected_error_message):

This comment has been minimized.

Copy link
@mrocklin

mrocklin Aug 19, 2019

Member

Small nit:

Rather than test that the entire error message is exactly this, we might instead test some properties of it that we care about, like that "network interface" and the names of other interfaces are present in the message.

If someone changes the error message but includes this kind of information I think that that's ok, and shouldn't be considered an error.

raise KeyError(
"{!r} is not a valid network interface. "
"Valid network interfaces are: {}".format(ifname, allowed_ifnames)
)

This comment has been minimized.

Copy link
@mrocklin

mrocklin Aug 19, 2019

Member

This is great. I really like changes like this!

@mrocklin

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Thanks for this change @lesteve ! I left one small nit, but I'm totally fine with this PR as-is.

I'm fine with changing the error type if you prefer.

@lesteve

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

I pushed a commit:

  • the type of the error is now ValueError
  • I reduced the test dependence on the specific wording of the error message. Now I am only testing that ifname, "network interface" and "lo" (or its OSX equivalent) are all in the error message (through a regex)

@mrocklin mrocklin merged commit 58b3abe into dask:master Aug 21, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mrocklin

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

Thanks @lesteve !

@lesteve lesteve deleted the lesteve:improve-get-ip-interface-error-message branch Aug 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.