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

Enable Pylint checks for PyWren #144

Merged
merged 10 commits into from
Aug 4, 2017
Merged

Enable Pylint checks for PyWren #144

merged 10 commits into from
Aug 4, 2017

Conversation

shivaram
Copy link
Collaborator

This PR addresses a bunch of remaining Pylint issues and for the ones which are not addressed, we add a local comment to disable Pylint. Finally this PR also adds a Pylint check into Travis.

@shivaram shivaram changed the title Enable PyLint checks for PyWren Enable Pylint checks for PyWren Jul 11, 2017
@shivaram
Copy link
Collaborator Author

@ooq This probably needs more closer review than the last one as it actually has some code changes. The travis check on the branch passes https://travis-ci.org/pywren/pywren/builds/252271798

@@ -158,17 +156,17 @@ def dump(self, obj):

def save_memoryview(self, obj):
"""Fallback to save_string"""
Pickler.save_string(self, str(obj))
Pickler.save_str(self, str(obj)) # pylint: disable=no-member

Choose a reason for hiding this comment

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

Should this be save_string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good qn - so if you see the code paths below, we call save_memoryview only for Python 3. And if you look at the _Pickler implementation in cpython [1] then you can see that the method is called save_str. The same file in the 2.7 branch [2] uses save_string

[1] https://github.com/python/cpython/blob/3.5/Lib/pickle.py#L698
[2] https://github.com/python/cpython/blob/2.7/Lib/pickle.py#L486

@@ -34,7 +34,7 @@ install:
- conda info -a
- conda create -q -n test-environment python=$TRAVIS_PYTHON_VERSION numpy pytest cython nose boto3 PyYAML Click pytest numba
- source activate test-environment
- pip install glob2
- pip install glob2 pylint tornado
Copy link
Collaborator

Choose a reason for hiding this comment

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

how was tornado installed before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was not installed before but we seemed to use it in cloudpickle.py[1] if it was available. The problem is lint doesn't like it when we import packages in _rebuild_tornado_coroutine.

[1]

def is_tornado_coroutine(func):

@@ -87,3 +88,4 @@ env:
- RUN_LAMBDA=true
- RUN_MACROREDUCE=true
- RUN_COMMANDLINE=true
- RUN_PYLINT=true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it's more resource-economic to run PyLint for all configurations, and halt the actual tests if linting fails? Essentially, passing linting should be a pre-requisite.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally, it would be nice if we can run RUN_PYLINT=true first, and make its passing be a dependence for RUN_LAMBDA=true, etc. But I don't think it's feasible with Travis?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can form a master script that does this - like they describe in https://docs.travis-ci.com/user/customizing-the-build/#Implementing-Complex-Build-Steps ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool. I think we can make a separate issue/PR for this.

@@ -173,10 +173,10 @@ def terminate_instances(instance_list):
# FIXME delete individuals
"""
for instance_name, instance_obj in instance_list:
logger.debug('Terminating instance %s', instance_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is from merging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No its actually trying to use instance_name to avoid an unused variable warning. i can also make it an _

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. It's cool.

for mod_path in list(mod_paths):
if module in mod_path and mod_path in mod_paths:
mod_paths.remove(mod_path)
if exclude_modules:

Choose a reason for hiding this comment

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

Should we be more explicit, with if exclude_modules is None?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We also dont want to run this loop if exclude_modules is an empty list ?

Choose a reason for hiding this comment

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

You're right. I'd assumed that a user would never pass in an empty list to exclude_modules, but that's not the case.

@@ -12,7 +12,7 @@
<https://web.archive.org/web/20140626004012/http://www.picloud.com/>`_.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if we'll need to understand the cloudpickle sources and maintain them in the future. If not, maybe we should just exclude them from linting completely? Diverging them too much from the original source might make it harder to sync with updated cloudpickle code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah this is a very good point. I thought about this while making changes as well. I guess we need to pull in @ericmjonas for this :)

@ericmjonas do you have an idea how many changes we made when we forked from cloudpickle ? Also do you see us syncing their more recent changes ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we will sync their more recent changes; is there a way to tell pylint to ignore certain files? In the interest of getting this out there I think we should merge this PR asap.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's doable? @shivaram
Let's then ignore those files, revert the pylint changes and merge this PR.

Copy link

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I'll do this tonight and merge after that

Copy link
Collaborator

@ooq ooq left a comment

Choose a reason for hiding this comment

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

This looks good to me. I have only one question w.r.t. how we should maintain modified external sources such as cloudpickle.py.

@shivaram
Copy link
Collaborator Author

shivaram commented Aug 4, 2017

@ooq I reverted the cloudpickle.py changes (it will look like a diff here because the earlier PR had some whitespace fixes and I also reverted those).

Waiting for Travis now and I guess somebody will need to approve this PR as well !

Copy link
Collaborator

@ooq ooq left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @shivaram ! Let's merge it as soon as tests pass.

@shivaram shivaram merged commit ba14fa2 into master Aug 4, 2017
@shivaram
Copy link
Collaborator Author

shivaram commented Aug 4, 2017

Thanks all ! This is merged now

@Vaishaal Vaishaal deleted the pylint-more-checks branch November 28, 2018 04:18
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

4 participants