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

[18.09] Don't attempt len(None), ensure stdout/stderr are strings in Job(Like).set_streams(). #6765

Merged
merged 1 commit into from
Sep 25, 2018

Conversation

natefoo
Copy link
Member

@natefoo natefoo commented Sep 25, 2018

Pulsar jobs that finish ok but with no stdout are failing with the following exception on usegalaxy.org:

Traceback (most recent call last):
  File "/cvmfs/main.galaxyproject.org/galaxy/lib/galaxy/jobs/runners/pulsar.py", line 514, in finish_job
    remote_metadata_directory=remote_metadata_directory,
  File "/cvmfs/main.galaxyproject.org/galaxy/lib/galaxy/jobs/__init__.py", line 1445, in finish
    job.set_streams(job.stdout, job.stderr)
  File "/cvmfs/main.galaxyproject.org/galaxy/lib/galaxy/model/__init__.py", line 227, in set_streams
    if (len(stdout) > galaxy.util.DATABASE_MAX_STRING_SIZE):
TypeError: object of type 'NoneType' has no len()

Possibly due to #6565 although I'm not sure how exactly.

@@ -222,8 +222,8 @@ def metrics(self):
return self.text_metrics + self.numeric_metrics

def set_streams(self, stdout, stderr):
stdout = galaxy.util.unicodify(stdout)
stderr = galaxy.util.unicodify(stderr)
stdout = galaxy.util.unicodify(stdout) or u''
Copy link
Member

@dannon dannon Sep 25, 2018

Choose a reason for hiding this comment

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

Might read cleaner to set default parameters on the function, any reason not to do that?

(Does unicodify return None for empty strings, instead of an empty unicode string? That's the only reason I could think of)

Copy link
Member

Choose a reason for hiding this comment

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

unicodify() returns None for None, not for ''

Copy link
Member

Choose a reason for hiding this comment

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

@nsoranzo Got it, yeah. I just realized my suggestion probably didn't make sense, but that's what I was getting at -- instead of having the logic in consumers like this it might be good to standardize behavior at a single point. That's for future refactoring though, I guess, no worries about it here.

@martenson martenson merged commit 529ed61 into galaxyproject:release_18.09 Sep 25, 2018
@natefoo
Copy link
Member Author

natefoo commented Sep 25, 2018

@martenson Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants