-
Notifications
You must be signed in to change notification settings - Fork 110
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
Save Proto Log #3225
Save Proto Log #3225
Conversation
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.
Had some initial feedback. Don't forget to remove the debugging logs once the PR is ready
LOG(DEBUG) << "I've got new: " << request.ai_config().ai_control_config().run_ai(); | ||
LOG(DEBUG) | ||
<< "I've got new: " | ||
<< request.ai_config().dribble_tactic_config().lose_ball_possession_threshold(); |
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.
remove this later
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.
bump
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.
sure.
# where we are saving the default configuration file | ||
DEFAULT_SAVE_DIRECTORY = "/tmp/tbotspython/thunderbots_configurations_proto" |
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.
put this in thunderscope.constants
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.
bump
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.
done.
:param proto_to_configure: The protobuf we would like to generate | ||
a parameter tree for. This should be | ||
a parameter tree for. This should be | ||
populated with the default values |
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.
it doesn't seem like you're passing proto_to_configure
anymore
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.
indeed. this is loaded by the file instead of passed by the parameter instead.
def __init__( | ||
self, proto_to_configure, on_change_callback, search_filter_threshold=60, | ||
self, on_change_callback, is_yellow, search_filter_threshold=60, | ||
): | ||
"""Create a parameter widget given a protobuf | ||
|
||
NOTE: This class handles the ParameterRangeOptions | ||
|
||
:param proto_to_configure: The protobuf we would like to generate |
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_yellow
isn't documented
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.
done.
""" | ||
|
||
# refreshing widgets after the parameters is called | ||
logging.info("I am updating widget?") |
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.
remove before merging
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.
sure.
logging.info("Something is changing?") | ||
logging.info(changes) | ||
|
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.
delete before merging
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.
sure.
@@ -106,6 +299,7 @@ def __handle_parameter_changed(self, param, changes): | |||
except (TypeError, NameError): | |||
exec(f"self.proto_to_configure.{child_name} = data") | |||
|
|||
logging.info(f"child name: {child_name} data: {data}") |
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.
delete before merging
save_button = QPushButton("Save Parameters") | ||
load_proto_button = QPushButton("Load Parameters") |
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.
ideally save and load would pop up a dialog box
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.
done.
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.
awesome work! I'm excited for this to be merged in
Co-authored-by: itsarune <42703774+itsarune@users.noreply.github.com>
…ware into save_proto_log
@Mr-Anyone If this PR is ready, you should make it "ready for review" so it's not a draft anymore. |
LOG(DEBUG) << "I've got new: " << request.ai_config().ai_control_config().run_ai(); | ||
LOG(DEBUG) | ||
<< "I've got new: " | ||
<< request.ai_config().dribble_tactic_config().lose_ball_possession_threshold(); |
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.
bump
# where we are saving the default configuration file | ||
DEFAULT_SAVE_DIRECTORY = "/tmp/tbotspython/thunderbots_configurations_proto" |
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.
bump
|
||
class ProtoConfigurationConstant: | ||
DEFAULT_SAVE_DIRECTORY = "/tmp/tbotspython/thunderbots_configurations_proto" | ||
DEFAULT_SAVE_PATH = DEFAULT_SAVE_DIRECTORY + "/default_conrfiguration.proto" |
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.
DEFAULT_SAVE_PATH = DEFAULT_SAVE_DIRECTORY + "/default_conrfiguration.proto" | |
DEFAULT_SAVE_PATH = DEFAULT_SAVE_DIRECTORY + "/default_configuration.proto" |
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.
bump. file name is misspelled
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.
Could you also pull master and run formatting? That should get CI running I think.
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.
I noticed the same issue that nima observed
I believe that the load button is not working. This is because I loaded everything but forgot to send the proto to unix_full_system. I've fixed. Build doesn't seem to be working. I am not quite sure if it is related to my changes. |
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.
Just tried the recent changes and saving, loading, and resetting seems to be working well!
|
||
class ProtoConfigurationConstant: | ||
DEFAULT_SAVE_DIRECTORY = "/tmp/tbotspython/thunderbots_configurations_proto" | ||
DEFAULT_SAVE_PATH = DEFAULT_SAVE_DIRECTORY + "/default_conrfiguration.proto" |
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.
bump. file name is misspelled
@@ -335,6 +335,25 @@ class TrailValues: | |||
DEFAULT_TRAIL_LENGTH = 20 | |||
DEFAULT_TRAIL_SAMPLING_RATE = 0 | |||
|
|||
|
|||
class ProtoConfigurationConstant: | |||
DEFAULT_SAVE_DIRECTORY = "/tmp/tbotspython/thunderbots_configurations_proto" |
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.
Actually I just realized that you're saving to /tmp
which is not persisted between computer restart. You should be saving in /opt/tbotspython/thunderbots_configurations_proto
instead
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.
that was intentional. since there is permission error if I were to put this in /opt
.
I think you would have to chown -R
the directory in setup.sh
if you were going to do that.
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.
How come we can save the layouts to /opt then?
Software/src/software/thunderscope/thunderscope.py
Lines 157 to 169 in 207a688
with shelve.open(filename, "c") as shelf: | |
for key, val in self.tab_dock_map.items(): | |
try: | |
shelf[key] = val.saveState() | |
except AttributeError: | |
pass | |
with shelve.open(LAST_OPENED_LAYOUT_PATH, "c") as shelf: | |
for key, val in self.tab_dock_map.items(): | |
try: | |
shelf[key] = val.saveState() | |
except AttributeError: | |
pass |
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 it works on your end, then I've no idea. It doesn't seem to work on my end. Here is the traceback.
===== 2024.07.10 10:07:27 =====
Traceback (most recent call last):
File "/home/vhe/.cache/bazel/_bazel_vhe/917dd48f905a75cc113dac63db2f3aa5/execroot/__main__/bazel-out
/k8-fastbuild/bin/software/thunderscope/thunderscope_main.runfiles/__main__/software/thunderscope/thun
derscope.py", line 140, in save_layout
pathlib.Path(SAVED_LAYOUT_PATH).mkdir(exist_ok=True)
File "/usr/lib/python3.8/pathlib.py", line 1288, in mkdir
self._accessor.mkdir(self, mode)
PermissionError: [Errno 13] Permission denied: '/opt/tbotspython/saved_tscope_layout'
===== 2024.07.10 10:07:27 =====
Traceback (most recent call last):
File "/home/vhe/.cache/bazel/_bazel_vhe/917dd48f905a75cc113dac63db2f3aa5/execroot/__main__/bazel-out
/k8-fastbuild/bin/software/thunderscope/thunderscope_main.runfiles/__main__/software/thunderscope/thun
derscope.py", line 140, in save_layout
pathlib.Path(SAVED_LAYOUT_PATH).mkdir(exist_ok=True)
File "/usr/lib/python3.8/pathlib.py", line 1288, in mkdir
self._accessor.mkdir(self, mode)
PermissionError: [Errno 13] Permission denied: '/opt/tbotspython/saved_tscope_layout'
I have pressed ctrl+s.
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.
Could you show me your ls -l
in the /opt
directory.
For me, it shows the following:
drwxr-xr-x 7 vhe vhe 4096 Feb 1 1980 gradle-8.8
drwxr-xr-x 10 vhe vhe 4096 Jun 18 18:38 idea
drwxr-xr-x 10 vhe vhe 4096 Jun 26 22:23 meld-3.22.2
drwxr-xr-x 3 root root 4096 Oct 7 2023 microsoft
drwxr-xr-x 2 vhe vhe 4096 Jul 2 18:04 shadowsocks
drwxr-xr-x 5 root root 4096 Jan 28 05:26 tbotspython
drwxrwxr-x 9 vhe vhe 4096 Jun 19 21:32 tomcat
drwxr-xr-x 6 root root 4096 Jun 15 17:11 wine-stable
the folder need root permission I think as a result. I think for you, it may be nima
instead of root
for the tbotspython directory. Do you still want me to change it?
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.
Mine seems to be owned by my user and not root.
~/thunderbots/Software/src (nima/fullsystem_proto_logger*) » ls -l /opt nima@nima-MS-7C87
total 36
drwxr-xr-x 4 root root 4096 Aug 16 2022 cisco
drwxr-xr-x 8 root root 4096 Apr 30 2021 clion-2019.3.5
drwxr-xr-x 8 root root 4096 Jan 8 2022 clion-2021.2.3
drwxr-xr-x 7 root root 4096 Nov 26 2023 docker-desktop
drwxr-xr-x 3 root root 4096 Apr 30 2021 google
drwxr-xr-x 2 root root 4096 Jul 16 2021 jetbrains-toolbox-1.21.9547
drwxr-xr-x 3 root root 4096 Dec 9 2023 microsoft
drwxrwxrwx 10 root root 4096 Apr 30 2021 STM32CubeMX_6.2.0
drwxr-xr-x 8 nima nima 4096 May 22 20:42 tbotspython
I just tried it and I can save layouts (ctrl + S
in thunderscope), but relaunching Thunderscope with it doesn't show anything from fullsystem anymore, though the layout is automatically loaded.
I guess if needed, we could update setup_software.sh
as well.
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.
I looked into it and we already do chown -R
in setup_software.sh
. For what ever reason that did not work for you, or maybe when you were testing with upgrading to Python 3.10 you manually recreated the /opt/tbotspython
python virtual env directory.
Software/environment_setup/setup_software.sh
Line 138 in 00372df
sudo chown -R $USER:$USER /opt/tbotspython |
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.
I think so. That could really be the case. Anyhow, I think I would just manually chown
my tbotspython directory for now.
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.
Okay. Could you update DEFAULT_SAVE_DIRECTORY
to use /opt
then?
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.
yeah, I will push an update now.
Co-authored-by: Nima Zareian <nimazareian80@gmail.com>
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.
Awesome work vincent!
Co-authored-by: itsarune <42703774+itsarune@users.noreply.github.com>
The following implements the following features.
Which essentially resolves #3195
Description
All of the feature is implemented in a widget. You could play around with it. Here is a screenshot:
Testing Done
Resolved Issues
resolves #3195
Length Justification and Key Files to Review
N/A
Review Checklist
Not done. This is only a draft PR!
.h
file) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom
. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO
(or similar) statements should either be completed or associated with a github issue