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

find nic_name automatically if misspecified #29

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

arberzela
Copy link

Added functionality to catch exceptions when the network card name is misspecified or is None. This may be useful in cluster environments with several submission nodes in cases when the network interfaces are different among them and the login node

@codecov-io
Copy link

Codecov Report

Merging #29 into development will decrease coverage by 0.39%.
The diff coverage is 0%.

Impacted file tree graph

@@              Coverage Diff               @@
##           development      #29     +/-   ##
==============================================
- Coverage        40.67%   40.27%   -0.4%     
==============================================
  Files               40       40             
  Lines             2611     2622     +11     
==============================================
- Hits              1062     1056      -6     
- Misses            1549     1566     +17
Impacted Files Coverage Δ
hpbandster/utils.py 0% <0%> (ø) ⬆️
hpbandster/core/master.py 91.66% <0%> (-2.5%) ⬇️
hpbandster/core/dispatcher.py 85.04% <0%> (-1.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f465db...358f591. Read the comment docs.

def get_nic_name_from_system():
import re
import subprocess
process = subprocess.Popen("ip route get 8.8.8.8".split(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this assume that the node has internet access? And it's not really portable as it requires ip to exist.
And pinging a Google DNS is not the most reliable way to figure out the network interface.


# if the network card name is not a valid one an ecxeption will be raised
# and the method get_nic_name_from_system will discover a valid card name
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the sentiment of your functionality, but I assumed that most users would call the nic_name_to_host function and catch the exception themselves in case they need to try more nic names because they are different across the cluster (which is usually not the case).

@sfalkner
Copy link
Collaborator

Hey Arber,
thanks for contributing. I think your functionality is very useful, but I don't think trying to guess the NIC should be the default behavior. Why don't you add a function guess_host that does what your code already does (maybe in a more portable way, maybe the socket module has the functionality to get the route to a given target).
Best,
Stefan

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

3 participants