Skip to content

Loading…

The constructor of Client() checks for AssertionError in validate_url to open a file instead of connection to a URL if it fails. #872

Closed
wants to merge 1 commit into from

2 participants

@temporaer

The constructor of Client() checks for AssertionError in validate_url to open a file instead of connection to a URL if it fails.

When running python -O, this failed, since the assertions were optimized away and other (random) errors were thrown.
These random errors are not catched. IMHO one should not depend on the side effects of assertions, which has been done here.

@minrk
IPython member

Fair point.

I overloaded the validate_url, which is really just supposed to check that a url is valid, but I used it in the client to distinguish between a url and a file, which is inappropriate, since assertions get optimized away.

Do you want to change the try/except in Client from:

try:
   util.validate_url(url_or_file)
except AssertionError:

to:

if not util.is_url(url_or_file):

adding to parallel.util:

def is_url(url):
    """boolean check for whether a string is a zmq url"""
    if '://' not in url:
        return False
    proto, addr = url.split('://', 1)
    if proto.lower() not in ['tcp','pgm','epgm','ipc','inproc']:
        return False
    return True

That way the assertions in validate_url are never used for actual logic, and only used for actual validation, where assertions are appropriate, and appropriate to optimize away when you trust that you don't have certain errors.

@temporaer

This definitely sounds cleaner than my (minimum-impact-solution-) patch, I'd go with your solution!

@minrk
IPython member

Did you want to make those changes to your PR, or shall I just make the change?

@temporaer

I changed my commit as you suggested.

@minrk

This needs to be util.is_url, otherwise it gets a NameError. Also, it's indented by a tab instead of 8 spaces for some reason.

@minrk
IPython member

Since it's a tiny typo, I'll go ahead and fix it in the merge. Thanks!

@minrk minrk added a commit that closed this pull request
@minrk minrk fix is_url typo in parallel client
closes #872
4332951
@minrk minrk closed this in 4332951
@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request
@minrk minrk fix is_url typo in parallel client
closes #872
75d5d76
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 17, 2011
  1. @temporaer

    introduce is_url() replacing validate_url() in cases where it is used…

    temporaer committed
    … in a control structure (as opposed to an assert)
Showing with 10 additions and 3 deletions.
  1. +1 −3 IPython/parallel/client/client.py
  2. +9 −0 IPython/parallel/util.py
View
4 IPython/parallel/client/client.py
@@ -306,9 +306,7 @@ def __init__(self, url_or_file=None, profile=None, profile_dir=None, ipython_dir
assert url_or_file is not None, "I can't find enough information to connect to a hub!"\
" Please specify at least one of url_or_file or profile."
- try:
- util.validate_url(url_or_file)
- except AssertionError:
+ if not is_url(url_or_file):
if not os.path.exists(url_or_file):
if self._cd:
url_or_file = os.path.join(self._cd.security_dir, url_or_file)
View
9 IPython/parallel/util.py
@@ -108,6 +108,15 @@ def asbytes(s):
s = s.encode('ascii')
return s
+def is_url(url):
+ """boolean check for whether a string is a zmq url"""
+ if '://' not in url:
+ return False
+ proto, addr = url.split('://', 1)
+ if proto.lower() not in ['tcp','pgm','epgm','ipc','inproc']:
+ return False
+ return True
+
def validate_url(url):
"""validate a url for zeromq"""
if not isinstance(url, basestring):
Something went wrong with that request. Please try again.