Proper test cleanup and resolve EOF Error#218
Conversation
Currently pytest hangs if a test fails, because resources are not cleaned up. Use a fixture to auto-clean during the teardown of the test.
Currently when creating a pipe, the server doesn't close the connection nor the created thread when the listening loop exits. This results to EOFError as we can see in the output when running the tests.
|
CI is failing because it gets a cached environment with these packages installed , maybe coming from the PR of updating the submodules to v5. |
thodkatz
force-pushed
the
fix-eof
branch
11 times, most recently
from
bb25012 to
791e998
Compare
- remove cleaning build files, `pip install -e` should be used - `pip install -e` doesn't work with multiple files, use a for loop to install the submodules
k-dominik
left a comment
k-dominik
left a comment
There was a problem hiding this comment.
Looking great, thank you for fixing this. I also appreciate the renamed variables, makes it easier to follow :)
Also really great that you added the proper cleanup in the rpc!
| def list_available_devices(self) -> List[IDevice]: | ||
| """ | ||
| List available devices on server | ||
| """ | ||
| return [device for device in self.list_devices() if device.status == DeviceStatus.AVAILABLE] | ||
|
|
||
| def list_reserved_devices(self) -> List[IDevice]: | ||
| """ | ||
| List reserved devices on server | ||
| """ | ||
| return [device for device in self.list_devices() if device.status == DeviceStatus.IN_USE] | ||
|
|
There was a problem hiding this comment.
I can see the value of adding these, but as far as I can tell they are not used. The commit message doesn't really explain why these have been added. So no context other than "nice to have" as far as I can see.
There was a problem hiding this comment.
it is being used here https://github.com/ilastik/tiktorch/pull/218/files#diff-31a22ff2b156f62383aa4c24cb4bf40cd11cf238819548c8c29dc51e7dd0d4a0R63, I wanted to assert that there aren't any reserved devices. Then we would have a function for getting all the devices, and one to get the reserved ones. For consistency, I have added to be able to get all the available ones as well, but it is true isn't used. I could remove it.
2 participants
As I am trying to migrate to github actions (#217), I have realized two issues when running the tests:
I have attempt to resolve these issue with their respective commits :)