-
Notifications
You must be signed in to change notification settings - Fork 61
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
[AEA-685] Gym connection fully async and full test coverage #1479
Conversation
|
||
self._queues = {} # type: Dict[str, asyncio.Queue] | ||
self._queue: Optional[asyncio.Queue] = None | ||
self._threaded_pool: ThreadPoolExecutor = ThreadPoolExecutor(10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for a high default of 10? Should also be declared as constant on top or be made configurable.
if self._queue is None: # pragma: nocover | ||
raise ValueError("Channel is not connected") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you wrap self._queue
in a property you can reduce some duplication of code..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, suggested some changes. There are also some failing tests related to the PR. Finally; please bump the version number of the connection and all its references.
9203bac
to
571f789
Compare
571f789
to
d8a35e2
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1479 +/- ##
===========================================
- Coverage 89.57% 88.14% -1.43%
===========================================
Files 242 242
Lines 17183 17188 +5
===========================================
- Hits 15391 15150 -241
- Misses 1792 2038 +246
Continue to review full report at Codecov.
|
@@ -8,7 +8,7 @@ aea_version: '>=0.5.0, <0.6.0' | |||
fingerprint: {} | |||
fingerprint_ignore_patterns: [] | |||
connections: | |||
- fetchai/gym:0.3.0 | |||
- fetchai/gym:0.4.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to bump the gym aea as a result
4f02dc9
to
79601d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Proposed changes
improvements and better tests coverage for gym connection
Fixes
#1468
Types of changes
Checklist
Put an
x
in the boxes that apply.develop
branch (left side). Also you should start your branch off ourdevelop
.aea cli
tool works