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

Add automated check for import style. #12110

Open
gabrieldemarmiesse opened this Issue Jan 23, 2019 · 16 comments

Comments

Projects
None yet
3 participants
@gabrieldemarmiesse
Copy link
Member

gabrieldemarmiesse commented Jan 23, 2019

In the directory keras/keras, we should only use relative imports for keras classes and funtions. In the directory keras/tests we should only use absolute imports.

The should be a tool to automatically run a check that those rules are respected and this tool should run in travis CI.

This is related to #11782.

@RaphaelMeudec

This comment has been minimized.

Copy link
Contributor

RaphaelMeudec commented Jan 23, 2019

@gabrieldemarmiesse Will look into this

@JoshuaRM

This comment has been minimized.

Copy link
Contributor

JoshuaRM commented Jan 30, 2019

Has this already been completed? If not I would like to work on this

@RaphaelMeudec

This comment has been minimized.

Copy link
Contributor

RaphaelMeudec commented Jan 30, 2019

@JoshuaRM Didn't have time to work on it yet, please go ahead!

@JoshuaRM

This comment has been minimized.

Copy link
Contributor

JoshuaRM commented Feb 1, 2019

@gabrieldemarmiesse Do you have any suggestions on how I can deal with only looking at import statements from keras classes and functions, as opposed to all imports in the file?
I am thinking about creating an ignore list of libraries which will not be checked as relative or absolute (future as an example)
Please let me know if you (or anyone else) have any suggestions

@JoshuaRM JoshuaRM referenced this issue Feb 7, 2019

Closed

Adding test_imports. Issue 12110 #12229

2 of 4 tasks complete
@JoshuaRM

This comment has been minimized.

Copy link
Contributor

JoshuaRM commented Feb 13, 2019

@gabrieldemarmiesse Originally when I approached this problem I looked into AST as the parse method is perfect for parsing, however it faced one large flaw. When parsing, the function parse() strips the . from the package/module before I can access it, making it impossible to test for relative imports that lead with . (in Import and ImportFrom format).

As far as I know I am unable to get around this because of the parse() function, which is what lead me to create my own parser in the previous PR. Please let me know if you have any suggestions, thanks!

@gabrieldemarmiesse

This comment has been minimized.

Copy link
Member Author

gabrieldemarmiesse commented Feb 13, 2019

I'm going to look into it. Does the object in the tree has any attribute which might indicate that it's a relative import?

@JoshuaRM

This comment has been minimized.

Copy link
Contributor

JoshuaRM commented Feb 13, 2019

As far as I am aware and have researched, there is no attribute for this. AST has attributes for Import and ImportFrom formats, but that is all.

@gabrieldemarmiesse

This comment has been minimized.

Copy link
Member Author

gabrieldemarmiesse commented Feb 13, 2019

Can you give the code? I'll look into it with a debugger to see if there is anything which can help.

@JoshuaRM

This comment has been minimized.

Copy link
Contributor

JoshuaRM commented Feb 13, 2019

This is the basic idea, using python function isinstance() with Import and ImportFrom attribute.

import ast
from collections import namedtuple

Import = namedtuple("Import", ["module", "name", "alias"])

def get_imports(path):
    with open(path) as fh:        
       root = ast.parse(fh.read(), path)

    for node in ast.iter_child_nodes(root):
        if isinstance(node, ast.Import):
            module = []
        elif isinstance(node, ast.ImportFrom):  
            module = node.module.split('.')
        else:
            continue

        for n in node.names:
            yield Import(module, n.name.split('.'), n.asname)
            
for i in get_imports("./imp.py"): print(i)

Using an example file called imp.py in the current directory

import something
from one import two
from .spy import six

The code from above yields this output

Import(module=[], name=['something'], alias=None)
Import(module=['one'], name=['two'], alias=None)
Import(module=['spy'], name=['six'], alias=None)

The last one is an example of the . being trimmed from modules as you can see the import was .spy
Please note that the name is trimmed in parse(), and the split method used in the code is not the cause of this problem

@JoshuaRM

This comment has been minimized.

Copy link
Contributor

JoshuaRM commented Feb 21, 2019

@gabrieldemarmiesse Have you had a chance to look at this yet?

@gabrieldemarmiesse

This comment has been minimized.

Copy link
Member Author

gabrieldemarmiesse commented Feb 21, 2019

No sorry about that, I've been fairly busy and I don't think I'll have the chance to take a look at it soon.

@JoshuaRM

This comment has been minimized.

Copy link
Contributor

JoshuaRM commented Mar 6, 2019

@gabrieldemarmiesse For the time being, I have separated my tool into its own repo and continuing development. It can be found at https://github.com/JoshuaRM/ImPyParser. In an effort to make it more robust as per request, I have encompassed it into a pip package also named ImPyParser. This can be installed simply through pip install ImPyParser. The tool is using pylint as well and prints the pylint errors found in the specified file/directory.

The pip package can be found at https://pypi.org/project/ImPyParser/

Please let me know if you have any questions or suggestions

@gabrieldemarmiesse

This comment has been minimized.

Copy link
Member Author

gabrieldemarmiesse commented Mar 6, 2019

I don't know if we can put this in the keras repo because it would increase maintenance cost. If you think it can be used without creating too many errors, you can add it to my bot and make it comment on pull requests. Here is the repo https://github.com/gabrieldemarmiesse/keras-bot This bot is currently used to comment on pull requests on keras contrib but I believe that it's easy to add functionalities with PyGithub. I'll add other functionalities in the future, like dropping a message when there are git conflicts for example. Your addition would be welcome if it's reliable. See keras-team/keras-contrib#416

@JoshuaRM

This comment has been minimized.

Copy link
Contributor

JoshuaRM commented Mar 13, 2019

I have taken the time to look into the bot and played around with it. The largest problem I am facing is reading the files, as they are hosted on the API. I am unable to read the files. I am testing using a testing repository with a PR structure identical to a keras one.

I have looked into the possibility of downloading the file, reading then deleting but even this is not possible.

Please let me know if there is a way to read the files or just access the current directory of the PR.

@gabrieldemarmiesse

This comment has been minimized.

Copy link
Member Author

gabrieldemarmiesse commented Mar 13, 2019

Maybe you can fetch the username and the branch name? Then you should be able to do a git clone and a git checkout.

@JoshuaRM

This comment has been minimized.

Copy link
Contributor

JoshuaRM commented Mar 14, 2019

Thank you for this suggestion, it was worked very well. The only problem is the inability to fetch a specific branch from a pull request. The only methods are get_branches() which returns a list of branches on the repo, and get_branch(key) which takes the repo name as a parameter. For testing purposes I have it hardcoded at the moment but will continue to look into this.

Additionally, I plan on adding an additional parameter to the send message function on the bot to include the import errors.

Please let me know your thoughts on these topics, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.