-
Notifications
You must be signed in to change notification settings - Fork 122
Enabling yellow bots to utilize an alternative Unix full system binary #3560
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
base: master
Are you sure you want to change the base?
Enabling yellow bots to utilize an alternative Unix full system binary #3560
Conversation
|
Did you know that for the Review Checklist, you can do [x] to have it show as a check instead. Basically get rid of the extra space |
Andrewyx
left a comment
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.
Left some comments, nice work so far
| friendly_colour_yellow=False, | ||
| should_restart_on_crash=False, | ||
| run_sudo=args.sudo, | ||
| running_in_realtime=(not args.ci_mode), | ||
| path_to_binary="software/unix_full_system", |
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.
note that blue will also need to be able to be adjusted
Andrewyx
left a comment
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.
Also, what triggered all of this formatting?
| blue_external_ai = "default" | ||
| yellow_external_ai = "default" |
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.
We can specify this as None type to avoid an edgecase for if someone's AI is named default
| if blue_external_ai == "default": | ||
| blue_path_to_binary = "software/unix_full_system" | ||
| else: | ||
| blue_path_to_binary = "/opt/tbotspython/external_ai/{}".format( | ||
| blue_external_ai | ||
| ) | ||
|
|
||
| if yellow_external_ai == "default": | ||
| yellow_path_to_binary = "software/unix_full_system" | ||
| else: | ||
| yellow_path_to_binary = "/opt/tbotspython/external_ai/{}".format( | ||
| yellow_external_ai | ||
| ) |
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 blue_external_ai == "default": | |
| blue_path_to_binary = "software/unix_full_system" | |
| else: | |
| blue_path_to_binary = "/opt/tbotspython/external_ai/{}".format( | |
| blue_external_ai | |
| ) | |
| if yellow_external_ai == "default": | |
| yellow_path_to_binary = "software/unix_full_system" | |
| else: | |
| yellow_path_to_binary = "/opt/tbotspython/external_ai/{}".format( | |
| yellow_external_ai | |
| ) | |
| default_full_system_path = "software/unix_full_system" | |
| external_full_system_dir = "/opt/tbotspython/external_ai" | |
| blue_path_to_binary = default_full_system_path if blue_external_ai is None else f"{external_full_system_dir}/{blue_external_ai}" | |
| yellow_path_to_binary = default_full_system_path if yellow_external_ai is None else f"{external_full_system_dir}/{yellow_external_ai}" |
|
Left some small nits |
This reverts commit 4d887e6.
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 your autoformatter just made a bunch of changes to this file irrelevant to this PR, I would suggest reverting them
| self.full_system = "{} --runtime_dir={} {} {}".format( | ||
| self.path_to_binary, | ||
| self.full_system_runtime_dir, | ||
| "--friendly_colour_yellow" if self.friendly_colour_yellow else "", | ||
| "--ci" if not self.running_in_realtime else "", |
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 for now this is fine cause we're using our own AI, but with other teams' the arguments such as --runtime_dir should be changed. I'm not exactly sure how we would handle that but thats for later to figure out
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 agree, we should add a todo to investigate this. We will probably need some additional common interface for recieving protos aside from unix sockets
| blue_external_ai = "None" | ||
| yellow_external_ai = "None" |
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 Andrew's suggestion above was to use None instead of a string. And then just change the if statements to use is None 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.
Yup, the built-in None type can be used here. It should be the responsibility of the Installer to set this value, and the most clear "default" type is I think None. Also, this might be personal preference, but I like to squash simple code like this using ternaries to reduce duplicated code. It lets readers glance past this code since its not really that important to the bigger picture of thunderscope
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.
Oops. That makes sense. Anyway, I think these two variables are just placeholders for now? When we integrate the backend we will make it read the toml file from external_ai.
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.
This file also has some python formatting changes, but I guess if our auto-formatter doesn't care its ok ?
Andrewyx
left a comment
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.
Almost there, left some more comments
| self.full_system = "{} --runtime_dir={} {} {}".format( | ||
| self.path_to_binary, | ||
| self.full_system_runtime_dir, | ||
| "--friendly_colour_yellow" if self.friendly_colour_yellow else "", | ||
| "--ci" if not self.running_in_realtime else "", |
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 agree, we should add a todo to investigate this. We will probably need some additional common interface for recieving protos aside from unix sockets
| should_restart_on_crash: bool = True, | ||
| run_sudo: bool = False, | ||
| running_in_realtime: bool = True, | ||
| path_to_binary: str = "software/unix_full_system", |
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.
A default value here should not exist if we are also setting it in thunderscope_main. Conceptually, this makes sense since it really shouldn't be the responsibility of this binary to decide alternative launch paths. Additionally, this would be a case of semantic coupling with the default path location in thunderscope (i.e. they are hard coded to be the same which is bad for refactoring)
| blue_external_ai = "None" | ||
| yellow_external_ai = "None" |
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.
Yup, the built-in None type can be used here. It should be the responsibility of the Installer to set this value, and the most clear "default" type is I think None. Also, this might be personal preference, but I like to squash simple code like this using ternaries to reduce duplicated code. It lets readers glance past this code since its not really that important to the bigger picture of thunderscope
Andrewyx
left a comment
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.
pending merge conflicts, but LGTM
|
@StarrryNight also if you could revert the formatting changes while you're sorting out the merge conflicts that would be great |
adrianchan787
left a comment
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.
Looks good! From a previous issue of mine I think Andrew said not to use wildcard imports (though Eric also mentioned code consistency which makes sense). Idk if they should be changed, but a lot of the imports aren't wildcard imports so maybe?
Yes that should be changed. But those where there from the start and is not related to the ticket so I think we might have to open a separate issue to remove most (if not all) wildcard imports |
39e1daf
annieisawesome2
left a comment
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.
looks good!
Description
This ticket enables yellow bots to utilize an alternative Unix full system stored locally.
File structure:
Binary Storage and Environment Setup:
Binaries for the unix_full_system are located in the /opt/tbotspython/external_ai directory. Both the tbotspython and external_ai folders are automatically generated during the initial environment configuration by running the './setup_software.sh' script. Once these directories are created, you can integrate new binaries by simply placing them directly into the external_ai folder.
Implementation and Execution Logic:
The behavior of the yellow bots is controlled by the external_library string variable, which is found on line 430 of 'software/thunderscope/thunderscope_main.py'. If this variable is set to "default", the yellow bots will operate using the same Unix system as the blue bots. However, if you assign any other string value to external_library, the system will search the external_ai directory for a binary with a matching filename and execute it specifically for the yellow bots.
If the search doesn't result in anything, I think the program fails. But this PR is supposed to integrate with user GUI within thunderscope so I will just leave this as is.
Testing Done
Tested by copying the unix_full_system binary from another branch, enemyThreats#3373, which resolves ticket #3373. The binary logs enemy threat rankings in thunderscope every frame.
By comparing the two team's LOGs in thunderscope, we can see if it works or not.
Video:
Screencast from 2026-01-11 17-04-12.webm
(TLDR: Blue doesn't log, yellow does, so it works.)
Resolved Issues
Resolves #3555
Length Justification and Key Files to Review
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.hfile) 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