Unify subprocess calls (to asyncio.create_subprocess_exec where possible) #464
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Just a note that we deprecated this plugin!
| return decorator | ||
|
|
||
|
|
||
| def async_exporter_job_wrapper(self, progress_start: int = 0, progress_end: int = 100): |
There was a problem hiding this comment.
This is a ask since you're working on this code. We don't need to have a "exported_job_wrapper" as everywhere else the pattern is to inherit the job_wrapper classes from the parent tlabplugin class. Can we get rid of the exporter_job_wrapper and just use the native ones in all export plugins?
There was a problem hiding this comment.
Maybe you missed updating the version number for this one?
| if num_gpus == 0 or not isnum(gpu_ids_formatted[0]): | ||
| num_gpus = torch.cuda.device_count() | ||
| gpu_ids = "" | ||
| async def __bootstrap(): |
There was a problem hiding this comment.
Just a nitpick but maybe we name this something else?
There was a problem hiding this comment.
Just a note we don't need to update version for this as we have deprecated this plugin, incase you end up incrementing for all missing ones
There was a problem hiding this comment.
Version number for this one too!
transformerlab/routers/plugins.py
Outdated
There was a problem hiding this comment.
Lets not do the hardware checks in this PR, I'll do a separate PR for these as we seem to change some core functionality and raise errors for rocminfo which is not the way it was intended
There was a problem hiding this comment.
Maybe we revert this file back
| tensorboard_process.terminate() | ||
| if tensorboard_process.returncode is None: # Process is still running | ||
| print("Stopping Tensorboard") | ||
| tensorboard_process.kill() |
There was a problem hiding this comment.
Is there a reason we changed this to kill()?
I think you mentioned some weird behaviour otherwise but just want to get it recorded on here
There was a problem hiding this comment.
yes, syncio.create_subprocess_exec does not play well with the kill() command, that's why it had to change to terminate()
| loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(loop) | ||
| loop.run_until_complete(run()) | ||
| loop.close() |
There was a problem hiding this comment.
Just a note that we have some db sync operations we used to run because of popen, maybe we change them to the async ones if necessary now that we do this change?
deep1401
dismissed
their stale review
Stashing my own review so Tony can provide a review once, I checked and everything works!
No description provided.