-
Notifications
You must be signed in to change notification settings - Fork 5
Draft: Asyncio conversion #148
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
base: main
Are you sure you want to change the base?
Conversation
This is the majority of the changes required to make the code asyncio complient and to switch to aioca.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #148 +/- ##
==========================================
- Coverage 99.23% 99.11% -0.12%
==========================================
Files 12 12
Lines 781 790 +9
==========================================
+ Hits 775 783 +8
- Misses 6 7 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Namely converting get_single()
Awaited on functions where required. I also had to switch to AsyncMock from MagicMock anywhere that a function needed to return a coroutine.
The special patter of making a class object return true or false using __bool__ is for sync code only
Some devices need awaiting and some dont
88b8a36 to
3ccbca6
Compare
| val = await device.get_value(handle, throw) | ||
| else: | ||
| val = device.get_value(handle, throw) | ||
| return val |
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.
This needs discussion on how to handle async vs non async devices.
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 we want to get an elements value, currently we must await it as it is possible that we need to do an async operation to get the data:
await element.get_value()
This then has to do a get_value on its data source manager, which again may have to do async work so must be awaited:
await data_source_manager.get_value()
This function then gets the data source which is non async, then it calls get_value on the datasource which must be awaited as the datasource could do async work:
await data_source.get_value()
Two examples of data sources are ATLatticeDataSource and SimpleDevice. Both are used by Virtac, when getting a value from an ATLatticeDataSource we must await a recalculation. When using SimpleDevice we are just getting stored data, so we dont need to do an await.
Currently these two cases are handled with: if inspect.iscoroutinefunction(device.get_value).
Maybe this could be improved?
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.
Is it an issue that we must await when getting a value even though it doesnt actually do any async actions? It will have a very slight computation overhead.
Most of our use cases are largely async. The main situation where we might not be is if someone loads an AT lattice and just wants to get the default values from it using pytac. They would have to await everything even though it is just reading data from the static lattice.
| if inspect.iscoroutinefunction(device.set_value): | ||
| await device.set_value(value, throw) | ||
| else: | ||
| device.set_value(value, throw) |
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.
Same here
Convert pytac from using cothread to aioca. This requires us to adopt asyncio.
This will be a breaking change for pytac and we will not support backwards compatibility.