Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Biologic driver changes for 0.2.x #78

Merged
merged 21 commits into from
May 22, 2024
Merged

Biologic driver changes for 0.2.x #78

merged 21 commits into from
May 22, 2024

Conversation

g-kimbell
Copy link

Several fixes to improve stability of jobs running on a Biologic MPG2.

The main changes are:

  1. Remove Filelock mechanism
    • This seemed to cause firmware unloading and did not prevent disconnects
  2. Retry connection if error is thrown
    • Many non-fatal errors were being thrown by the Biologic API calls which can be recovered from by retrying
  3. Check "RUN" or "STOP" status from get_data not get_status
    • get_status sometimes silently failed and returned all 0 instead of an error
    • This was interpreted as a "STOP" command which ended the job
    • Now we use get_data which reduces the number of API calls needed, and should not silently fail
    • We also check for several "STOP" statuses in a row before stopping, though this may not be necessary now
  4. cancel_all function in ketchup
    • Cancel all running and queued jobs
    • This was mostly for testing purposes as we were submitting and cancelling many jobs

@g-kimbell
Logging bug fix
0692724 
Move logger message to avoid using dt variable before it is defined
@g-kimbell
Fix: API random disconnect issue
df842f3 
- Implement KBIO_api_wrapped class for use in 'with' statements to handle connection errors.
- Adjust get_status, get_data, start_job, and stop_job methods to attempt 120 times before failure.
- Reduce API connection timeout to 1 second.
- Remove filelock as it did not help with disconnect, caused other issues.
- Add logging to help debugging of disconnect problem.

The jobs are now more robust against errors.

Known issue: we have seen the API disconnect with a "successful" exit code much earlier than expected.
It is still not clear why this is happening - further investigation needed.
@g-kimbell
Fix: data_poller stops too early without an error
872eac5 
- get_status randomly returns 0 for all attributes when there are many api calls
- add check for firmware version to see if get_status is working correctly, otherwise retry
- refactor the data_poller loop
- use get_data instead of get_status for checking run status in data_poller loop

Additionally:
- add warning for channel memory saturation
- lower several logging severity to DEBUG
- give log message when stopping data polling
@g-kimbell
Implement cancel_all function
64b6891
@g-kimbell
Improve legibility of logging messages for get_data
b82d07f
@g-kimbell
Ensure many "STOP" statuses are returned consecutively before stoppin…
09b2f10 
…g polling
@g-kimbell
Don't overwrite cancel parser
897cdc0 
Does not change anything functionally, but looks more sensible.
@g-kimbell
Black formatting
7bcb025
@g-kimbell
Reduce number of consecutive STOP statuses needed to stop
8c6a088
PeterKraus
PeterKraus requested changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

@PeterKraus PeterKraus left a comment

Choose a reason for hiding this comment

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

Hello Graham, looks nice. I didn't give it a super-thorough read (that will have to wait until the end of next week), but this should give you something to do in the meantime, if you have the time.

You should also add yourself to the code contributors in the toplevel README.md, in the toplevel setup.py file, and also here:

tomato/src/tomato/drivers/biologic/__init__.py

Lines 97 to 98 in badda59

.. codeauthor::
Peter Kraus

Also, please, run the codebase through black.

src/tomato/drivers/biologic/main.py Show resolved Hide resolved
src/tomato/drivers/biologic/main.py Outdated Show resolved Hide resolved
src/tomato/drivers/biologic/main.py Outdated Show resolved Hide resolved
src/tomato/drivers/biologic/main.py Outdated Show resolved Hide resolved
src/tomato/drivers/driver_funcs.py Outdated Show resolved Hide resolved
src/tomato/drivers/biologic/main.py Show resolved Hide resolved
src/tomato/drivers/biologic/main.py Outdated Show resolved Hide resolved
@g-kimbell
Add Graham Kimbell to code authors
1d29ee8
@g-kimbell
Completely remove Filelock
138e7cf 
Remove from setup, remove lockpath from function signatures, remove from config requirements
@g-kimbell
Change time.time() to time.perf_counter()
12630eb
@g-kimbell
Use lazy string formatting for logs, raise errors caught
8fb65f3 
Also moved connection logs to __enter__ and __exit__ functions of KBIO_api_wrapped
@PeterKraus PeterKraus changed the title 0.2.x Biologic driver changes for 0.2.x May 15, 2024
PeterKraus
PeterKraus requested changes May 15, 2024
View reviewed changes
Copy link
Contributor

@PeterKraus PeterKraus left a comment

Choose a reason for hiding this comment

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

Hi @g-kimbell, nice work, just a few small things, shouldn't take too long.

docs/source/quickstart.rst Outdated Show resolved Hide resolved
src/tomato/drivers/biologic/main.py Outdated Show resolved Hide resolved
src/tomato/drivers/biologic/main.py Outdated Show resolved Hide resolved
src/tomato/drivers/biologic/main.py Outdated Show resolved Hide resolved
src/tomato/drivers/driver_funcs.py Outdated Show resolved Hide resolved
PeterKraus
PeterKraus approved these changes May 21, 2024
View reviewed changes
Copy link
Contributor

@PeterKraus PeterKraus left a comment

Choose a reason for hiding this comment

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

Great work, thanks. Let me know if you think it's ready to merge or if there's anything else that you'd like to do here!

@g-kimbell
Copy link
Author

Hi Peter, great, I think this is ready to merge then.
I have another branch where I have added the ability for ketchup submit to receive json strings directly rather than a file, and ketchup status to spit out a json string instead of the formatted table, to make some remote work easier: https://github.com/g-kimbell/tomato/tree/0.2.x-json-ketchup - it is useful for me to submit jobs/check statuses remotely with the ability to easily submit e.g. 36 different jobs on 36 different cells.

@PeterKraus PeterKraus merged commit bad20cd into dgbowl:0.2.x May 22, 2024
@PeterKraus
Copy link
Contributor

I have another branch where I have added the ability for ketchup submit to receive json strings directly rather than a file, and ketchup status to spit out a json string instead of the formatted table

Feel free to open a new PR into the same branch, if you'd like it in the tomato-0.2.3 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PeterKraus PeterKraus PeterKraus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@g-kimbell @PeterKraus