Skip to content

Modify async to async_ to support Python 3.7#286

Merged
toslunar merged 3 commits intochainer:masterfrom
mmilk1231:master
Jul 11, 2018
Merged

Modify async to async_ to support Python 3.7#286
toslunar merged 3 commits intochainer:masterfrom
mmilk1231:master

Conversation

@mmilk1231
Copy link
Copy Markdown
Contributor

async becomes a keyword in Python 3.7. Thus when I import ChainerRL with Python 3.7, the following SyntaxError happened:

  File "/usr/local/lib/python3.7/site-packages/chainerrl-0.3.0-py3.7.egg/chainerrl/agents/a3c.py", line 122
    async.assert_params_not_shared(self.shared_model, self.model)
         ^
SyntaxError: invalid syntax
...

This PR avoid the error by modifying async to async_.

References about Python 3.7:
https://www.python.org/dev/peps/pep-0492/
https://docs.python.org/3/whatsnew/3.7.html#changes-in-python-behavior

self.q_function = copy.deepcopy(self.shared_q_function)

async.assert_params_not_shared(self.shared_q_function, self.q_function)
async_.assert_params_not_shared(self.shared_q_function, self.q_function)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you break this line (>79 chars)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry for my mistakes. I break this line in the new commit.

Copy link
Copy Markdown
Member

@toslunar toslunar left a comment

Choose a reason for hiding this comment

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

Thanks a lot for sending the PR. This fixes #286. The new name async_ looks good to me.

Could you also fix imports in tests? (chainerrl/tests/misc_tests/test_async.py)

@mmilk1231
Copy link
Copy Markdown
Contributor Author

I fixed chainerrl/tests/misc_tests/test_async.py by rewriting async to async_.
Thank you for pointing it out.

Copy link
Copy Markdown
Member

@toslunar toslunar left a comment

Choose a reason for hiding this comment

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

LGTM

@toslunar toslunar merged commit f6c3779 into chainer:master Jul 11, 2018
@toslunar toslunar mentioned this pull request Jul 13, 2018
@muupan muupan added this to the v0.4 milestone Jul 23, 2018
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.

3 participants