-
Notifications
You must be signed in to change notification settings - Fork 394
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
Fix python script and allow autotune prep/core to write to file directly #1394
Conversation
3d83eae
to
04d3d46
Compare
logging.info('Writing output to {filename}'.format(filename=autotune_run_filename)) | ||
|
||
# Autotune (required args, <autotune/glucose.json> <autotune/autotune.json> <settings/profile.json>), | ||
autotune_prep = 'oref0-autotune-prep {ns_treatments} {profile} {ns_entries} {pump_profile} --output-file {autotune_run_filename}'.format(ns_treatments=ns_treatments, profile=profile, ns_entries=ns_entries, pump_profile=pump_profile, autotune_run_filename=autotune_run_filename) |
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.
pumpprofile.json
is a required argument to autotune-prep
. Though FWICT it's the same as profile.json
so I'm not sure...why it's required here.
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.
The pumpprofile.json represents the pump's settings. Once autotune has been run, the profile.json should get updated to reflect the autotuned settings, but the pumpprofile.json should keep the pump's settings in order to anchor the range over which autotune can safely adjust settings.
|
||
|
||
DIR = '' |
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.
IMO we shouldn't be using global variables here.
@@ -0,0 +1,3 @@ | |||
requests==2.25.1 |
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.
pin python dependencies.
Has anyone tested this branch, such as by running a fresh install from it on a rig? |
@hsghori did you see my question above about testing? |
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.
Installed this on a rig and manually ran oref0-cron-nightly successfully.
While running the autotune python script i noticed a couple issues:
bin/oref0-autotune.py
was not compatible with python3.bin/oref0-autotune-core.js
andbin/oref0-autotune-prep.js
is piped to an output file which was causing autotune to crash because unrelated logs were being piped to the output file.This PR introduces py3 compatibility (and fixes a couple of style issues) and adds an optional output file argument to the autotune core and prep modules to prevent extraneous data from being written to the files.