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

Pylint cleanup #143

Merged
merged 6 commits into from Jul 8, 2017
Merged

Pylint cleanup #143

merged 6 commits into from Jul 8, 2017

Conversation

@shivaram
Copy link
Collaborator

@shivaram shivaram commented Jul 3, 2017

This PR contains changes to address warnings from Pylint. Right now this includes removing trailing whitespace and organizing the imports.

@shivaram
Copy link
Collaborator Author

@shivaram shivaram commented Jul 3, 2017

There are still some more warnings left. The current output of running pylint --rcfile ./pylintrc pywren is at https://gist.github.com/shivaram/039511355e726ea7a277f51d3032e1ac
Some of these warnings are simple to fix (like the print formatting).

The other warnings of relative import seem more tricky to fix as from my testing we rely on relative imports to get the lambda runner to work correctly.

Also @ooq @ericmjonas it would be good if you could look at the list of disabled checks at https://github.com/pywren/pywren/blob/pylint/pylintrc#L84 and see if something should be enabled.

@shivaram
Copy link
Collaborator Author

@shivaram shivaram commented Jul 4, 2017

Ok I enabled a bunch of more rules and fixed issues raised in them. https://gist.github.com/shivaram/5e297f237fabd268b860f367fb2f9331 is the latest remaining errors.

Some of the remaining ones are about unused variables which I am a bit hesitant to change in this PR as that will need more careful reviewing ?

@ooq
Copy link
Collaborator

@ooq ooq commented Jul 7, 2017

You have more commits on this, right? @shivaram

@shivaram
Copy link
Collaborator Author

@shivaram shivaram commented Jul 7, 2017

I will rebase this with @Vaishaal's recently merged PR. I have some more commits to follow that make some more code changes -- Should we do this in this same PR or do it in a follow up PR ?

@ooq
Copy link
Collaborator

@ooq ooq commented Jul 7, 2017

I think the idea of having a "safe" PR that requires minimal code change, and a follow up PR that finalizes lint, is pretty reasonable. We can pass the first one very quickly.

@shivaram
Copy link
Collaborator Author

@shivaram shivaram commented Jul 7, 2017

Great. Then I will stash the other commits. Can you take a look at the existing ones ? I'll just push one more commit with the rebase

shivaram added 5 commits Jul 3, 2017
This includes removing trailing whitespace, removing unused imports
and sorting imports
@shivaram shivaram force-pushed the pylint branch from 774d78e to 1f9e2f2 Jul 7, 2017
@shivaram
Copy link
Collaborator Author

@shivaram shivaram commented Jul 7, 2017

Seems like a flaky test run - Value (pywren_travis_589_7_052493) for parameter iamInstanceProfile.name is invalid. Invalid IAM Instance Profile name - Re-triggering the tests

@shivaram
Copy link
Collaborator Author

@shivaram shivaram commented Jul 7, 2017

@ooq - tests pass now

@ooq
ooq approved these changes Jul 7, 2017
Copy link
Collaborator

@ooq ooq left a comment

Looks all good, except one comment on how SQSInvoker should be imported. Thanks for the PR, it's quite some lines!

@@ -60,7 +59,7 @@ def remote_executor(config=None, job_max_runtime=3600):

AWS_REGION = config['account']['aws_region']
SQS_QUEUE = config['standalone']['sqs_queue_name']
invoker = invokers.SQSInvoker(AWS_REGION, SQS_QUEUE)
invoker = queues.SQSInvoker(AWS_REGION, SQS_QUEUE)

This comment has been minimized.

@ooq

ooq Jul 7, 2017
Collaborator

Is this caused by import changes due to lint? I feel previous code was cleaner in that all invokers are from invokers.py.

This comment has been minimized.

@shivaram

shivaram Jul 8, 2017
Author Collaborator

I'm not sure how this was working before. From what I can see SQSInvoker is only present in queues.py ? So was it somehow being imported by invokers.py and then re-exported ? I could try to revert this and see if lint and tests are happy, but I'm also curious what the intended behavior here is

This comment has been minimized.

@ooq

ooq Jul 8, 2017
Collaborator

I think the invokers should be a module and the queues.py just provide some implementations. So it's nice to access all invokers from invokers.py.
Personally I feel having a separatequeues.py file is not very necessary for now, but we might expand more in the future.

This comment has been minimized.

@ooq

ooq Jul 8, 2017
Collaborator

Does lint complain if you revert back?

This comment has been minimized.

@shivaram

shivaram Jul 8, 2017
Author Collaborator

So to do this I think we need to import SQSInvoker into invokers.py ? If I do that then I get W: 9, 0: Unused SQSInvoker imported from pywren.queues (unused-import) in invokers.py.

If I dont do that import then I get E: 61,14: Module 'pywren.invokers' has no 'SQSInvoker' member (no-member)

Again -- I'm not sure having the class in queues.py is a good idea if we want to call it from invokers.py (if we need a module we need to create invokers as a directory ? that seems overkill)

This comment has been minimized.

@ooq

ooq Jul 8, 2017
Collaborator

Agree that would overkill.
Another way is to wrap SQSInvoker in invokers.py (We probably want to change the naming as well, having a invoker inside a queue.py is not ideal). But this should not be in this PR.
I'm fine with merging.

This comment has been minimized.

@shivaram

shivaram Jul 8, 2017
Author Collaborator

Ok cool. I just fixed one minor thing due to a merge conflict with vaishaal's PR. Feel free to merge once the tests pass !

@shivaram shivaram merged commit 9d8096c into master Jul 8, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/push The Travis CI build passed
Details
@Vaishaal Vaishaal deleted the pylint branch Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.