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

Fix error #2507 #2509

Merged
merged 1 commit into from
Sep 1, 2018
Merged

Fix error #2507 #2509

merged 1 commit into from
Sep 1, 2018

Conversation

nryanov
Copy link
Contributor

@nryanov nryanov commented Aug 28, 2018

Description

Replace direct attribute accessing by using built-n function getattr

Motivation and Context

Fixes #2507

Have you tested this? If so, how?

I tried to reproduce this issue without and with this patch. As local test showed, this patch fix this issue

@@ -822,7 +822,7 @@ def add_task(self, task_id=None, status=PENDING, runnable=True,
task.family = family
if not getattr(task, 'module', None):
task.module = module
if not task.param_visibilities:
if not getattr(task, 'param_visibilities', None):
Copy link
Contributor

Choose a reason for hiding this comment

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

hasattr(task, 'param_visibilities') is more concise with the same effect, but either one works.

Copy link
Contributor Author

@nryanov nryanov Aug 28, 2018

Choose a reason for hiding this comment

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

There is a chance that task will have param_visibilities equal to {} by default and in this case hasattr may return True, but expected result was False.
So, just to avoid additional check i've decided to use getattr

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. Stick with getattr. Thanks!

@rayrrr
Copy link
Contributor

rayrrr commented Aug 28, 2018

Approved, though you will have to figure out why the Travis build is failing and take appropriate action.

@nryanov
Copy link
Contributor Author

nryanov commented Aug 28, 2018

There is a lot of failed S3 tests. I'm not sure that this change has broken it, but i'll re-check it anyway

@dlstadther
Copy link
Collaborator

No specific luigi PR appears to be the cause of the s3 test failures. I suspect it has to do with a dependency update. I've take a couple minutes and found that boto3 and botocore versions increased between passing and failing builds. But don't have more time to dedicate.

Any and all efforts provided to identify and resolve this issue will be most appreciated!

Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

S3 test failures were a Travis issue that has since ceased.

This LGTM

@dlstadther dlstadther merged commit 5d99d60 into spotify:master Sep 1, 2018
@rayrrr
Copy link
Contributor

rayrrr commented Sep 1, 2018

👏 👏 👏

@nryanov nryanov deleted the fix_2507 branch September 1, 2018 18:14
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.

502 Bad Gateway error tied to param visibility
3 participants