SIM RFC: AI selector backend #3569

adrianchan787 · 2026-01-17T18:27:53Z

Description

Created function that returns a list of all available functions in external/ai
Created function that persists binaries (passed in arguments) to a toml file
Creates function that reads from toml file
HAVE NOT integrated yet; I think it might be easier for me to do that after SIM RFC: AI selector interface #3568 and Enabling yellow bots to utilize an alternative Unix full system binary #3560 are merged?

Testing Done

- Created a test folder test_external_ai in /opt/tbotspython/ (and later deleted it); tested the functions (albeit pretty shallowly) there and it seems to work

Resolved Issues

Link any issues that this PR resolved.

resolves #3557

Length Justification and Key Files to Review

Review Checklist

It is the reviewers responsibility to also make sure every item here has been covered

[] Function & Class comments: All function definitions (usually in the .h file) should have a javadoc style comment at the start of them. For examples, see the functions defined in thunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.
[] Remove all commented out code
[] Remove extra print statements: for example, those just used for testing
[] Resolve all TODO's: All TODO (or similar) statements should either be completed or associated with a github issue
(draft PR)

GrayHoang · 2026-01-17T19:36:50Z

src/software/thunderscope/binary_context_managers/ai_selector.py

+    file_path = os.path.join(AISelectorConstants.AI_FOLDER, selected_ai)
+    # Default to our full system if it is selected or the selected binary doesn't exist
+    if selected_ai == AISelectorConstants.FULLSYSTEM_DISPLAY_NAME or not os.path.isfile(file_path):
+        return AISelectorConstants.FULLSYSTEM_PATH


Do we want to log some flag to indicate that our AI is being used instead of the specified one? It may not always be immediately clear if we are using the correct AI, and we should want to know when we have failed to specify the correct one.

That makes sense imo. Do you think we should only do this when we read/load the AI in, or when we write as well? I was thinking there's not really any point to do it when we write to the TOML, but I'm not sure.
Also, I'm using the logging module for this, would like to clarify if that's right?

src/software/thunderscope/binary_context_managers/BUILD

src/software/thunderscope/thunderscope_main.py

StarrryNight · 2026-01-19T17:33:53Z

src/software/thunderscope/binary_context_managers/ai_selector.py

+
+    # Return a dictionary of colour to the path string
+    saved_ais = {
+        "Blue": saved_blue,


Or here we directly use AISelectorConstants.X_TOML_FIELD as keys

(wait sorry i'll reply to the other comments in a bit, ty for catching all these) yeah that makes a lot of sense, code would be a lot cleaner too. i'll keep it as a dictionary and do this
(oh lmao didn't see the comment under this, nvm i'll do that instead)

StarrryNight · 2026-01-19T17:46:24Z

src/software/thunderscope/binary_context_managers/ai_selector.py

+    else:
+        # Remove all white space and return
+        #TODO not sure if that is necessary, but probably doesn't hurt?
+        return "".join(file_path.split())


We can remove only leading and trailing whitespaces by using .split(), just to prevent the whitespaces in the path from being removed.
I don't think this is necessary, because we generated the path in the code by combining AI_FOLDER and filename. However I think its good to be more defensive and leave it in. So if somebody edits the TOML file and leaves a white space we can catch it.

That's what I was thinking too, ty. I'll just remove leading and trailing whitespaces for now

StarrryNight · 2026-01-19T17:47:07Z

src/software/thunderscope/binary_context_managers/ai_selector.py

+        "Yellow": saved_yellow
+    }
+
+    #TODO not sure if this should be a dictionary or something like a tuple


A dictionary works. I think the code will be more clean and safe if we use a new data class (say AIConfig) with two fields instead of a dictionary, and we pass the object. This also allows us to save configs more easily (if we need to in the future).

StarrryNight · 2026-01-19T17:48:12Z

src/software/thunderscope/constants.py

+
+    AI_FOLDER = "/opt/tbotspython/external_ai"
+    CONFIG_FILE_PATH = AI_FOLDER + "/ai_config.toml"
+    FULLSYSTEM_DISPLAY_NAME = "Default Fullsystem" #TODO is there a better name?


This is fine

StarrryNight · 2026-01-19T17:49:03Z

src/software/thunderscope/constants.py

+    AI_FOLDER = "/opt/tbotspython/external_ai"
+    CONFIG_FILE_PATH = AI_FOLDER + "/ai_config.toml"
+    FULLSYSTEM_DISPLAY_NAME = "Default Fullsystem" #TODO is there a better name?
+    FULLSYSTEM_PATH = "software/unix_full_system" #TODO is this right? Should I use absolute path?


Not sure. For my part software/unix_full_sytem works but I think its more safe to have an absolute path. I'm not sure how to do it for bazel though so perhaps someone else can answer this.

ig the main reason for the selector is to initialize thunderscope_main so if it works for you, i think it should work? i'll keep it for now

StarrryNight · 2026-01-19T17:49:50Z

left some comments

StarrryNight · 2026-01-19T17:53:15Z

Also the autoref game test is failing because you will need to add ai_selector.py to the as a dependency to the thunderscope_main.py build target

nycrat

Hey Adrian, I left like a bunch of nits, but one main thing is that we are using Runtime instead of AI, so you should rename some of the functions and variables accordingly.

Also there is an issue in the RuntimeLoader file where cached_runtimes is being used incorrectly.

Other than that, it generally looks good

src/software/thunderscope/binary_context_managers/runtime_manager.py

environment_setup/setup_software.sh

src/software/thunderscope/binary_context_managers/runtime_loader.py

nycrat · 2026-01-23T02:10:08Z

src/software/thunderscope/binary_context_managers/runtime_loader.py

+        # Display logging message when using default FullSystem
+        if config.chosen_blue_name == RuntimeManagerConstants.RUNTIME_CONFIG_PATH:
+            logging.info("TBots FullSystem selected for blue side.")
+        if config.chosen_yellow_name == RuntimeManagerConstants.RUNTIME_CONFIG_PATH:
+            logging.info("TBots FullSystem selected for yellow side.")


nit: I don't think this logging is necessary, would like to hear from others

nycrat · 2026-01-23T02:11:50Z

src/software/thunderscope/binary_context_managers/runtime_loader.py

+                file.write(selected_ais)
+        except (PermissionError, NotADirectoryError):
+            logging.error("Could not access the configuration file.")
        pass


Get rid of pass, its only used in Python as a placeholder

nycrat · 2026-01-23T02:17:55Z

src/software/thunderscope/binary_context_managers/runtime_loader.py

    def _create_runtime_config(self) -> None:
        """Creates the runtime configuration file on disk and throws an error upon failure."""
+        # TODO Not sure if this function really has an use, since python open() creates a new file by default if it
+        #  doesn't exist
        pass


I agree, I think this can just be deleted

nycrat · 2026-01-23T02:20:17Z

src/software/thunderscope/binary_context_managers/runtime_loader.py

+        selected_ais = (
+            f'{RuntimeManagerConstants.RUNTIME_CONFIG_BLUE_KEY} = "{blue_path}"\n'
+            f'{RuntimeManagerConstants.RUNTIME_CONFIG_YELLOW_KEY} = "{yellow_path}"\n'
+        )


nit: Is there a function in the toml library that serializes a dict so that we don't need to use fstrings to manually do it?

tomllib is only used for reading, and I couldn't find a easier module that supported writing (I think most of the options were third party software and I didn't really want to do that).

nycrat · 2026-01-23T02:28:57Z

src/software/thunderscope/binary_context_managers/runtime_loader.py

+            logging.info("TBots FullSystem selected for yellow side.")
+
+        # Cache runtimes
+        self.cached_runtimes = config


NOTE: this cached_runtimes is being used in two different ways which causes an error. There should be a separate cached_runtimes and cached_config.

Oh yikes not sure what I was thinking there lmao, tysm

nycrat

Functionality seems good 👍

And I know this isn't a part of this PR, but I would just note that there is a "Current Commit" and "localhost" option in the UI which both represent the default unix_full_system binary. I think the "Current Commit" option which is added in gl_gamecontroller_toolbar.py should be taken out.

nycrat · 2026-01-23T19:14:34Z

src/software/thunderscope/binary_context_managers/runtime_loader.py

+            )
+
+        # Check if paths are valid runtimes
+        if not self._is_valid_runtime(config.chosen_blue_path):


True, I think it makes sense here, and also log the selected binary that couldn't be found

src/software/thunderscope/binary_context_managers/runtime_loader.py

nycrat · 2026-01-24T00:05:01Z

src/software/thunderscope/binary_context_managers/runtime_loader.py

+        if os.path.isfile(runtime_path) and os.access(runtime_path, os.X_OK):
+            return True
+        return False


nit: can just return the expression in the if statement since it evaluates to a boolean anyways

Andrewyx · 2026-01-24T00:43:26Z

environment_setup/setup_software.sh

+sudo touch /opt/tbotspython/external_runtimes/runtime_config.toml
 sudo chown -R $USER:$USER /opt/tbotspython/external_runtimes/

+sudo chmod 600 /opt/tbotspython/external_runtimes/runtime_config.toml


can we just make this file dynamically and give perms for the entire folder? Is this a good idea @Lmh-java @sauravbanna ? I'm not that knowledgeable on Linux perms

yeah not sure if this line is necessary, the toml library will probably make the file if not exists anyway upon first write.

Oh on an related note, would either of you happen to know if making the toml file without sudo chmod will make it non-executable? I couldn't find any good resources, though a week ago I did (manually) make a external runtime folder in opt/tbotspython to test it out and iirc it was executable (though I don't think this test reflects the setup script in tbots). (I was thinking of making it non-executable so I could filter it out when fetching the list of runtimes)

I have limited context, but
When you touch to create a file, it creates a file with permission 644 and the owner being the current user (in this case sudo).
Line 149 transfers the ownership of the file to the current user ($USER). So now it is 644 under the current user. 644 means the file owner has read and write access and other users will have read access.

Line 151 makes the file permission mask to be 600, which get rids of the access of other users, but the current user can still read and write. Therefore, line 151 might not be needed.

Iirc, if you give the permission to the entire folder and make the folder owned by the current user, then files created in that folder later will have no problems. But I think you might need more experiments on that. I am only 70% sure.

Makes sense ty, got rid of the stuff in setupsoftwaresh then

sauravbanna · 2026-01-24T03:45:21Z

environment_setup/setup_software.sh

+sudo touch /opt/tbotspython/external_runtimes/runtime_config.toml
 sudo chown -R $USER:$USER /opt/tbotspython/external_runtimes/

+sudo chmod 600 /opt/tbotspython/external_runtimes/runtime_config.toml


yeah not sure if this line is necessary, the toml library will probably make the file if not exists anyway upon first write.

sauravbanna · 2026-01-24T03:50:47Z

src/software/thunderscope/binary_context_managers/runtime_loader.py

+                    RuntimeManagerConstants.RUNTIME_CONFIG_BLUE_KEY
+                    in selected_runtime_dict.keys()
+                ):
+                    config.chosen_blue_path = selected_runtime_dict[


since we start with the default blue/yellow fullsystem paths anyway in the RuntimeConfig() object, why not just check self._is_valid_runtime here itself? so we only update the runtime path if it exists + is a valid runtime, avoiding setting the config paths back to default in lines 109-112

That makes a lot of sense, I'll do that ty

Andrewyx · 2026-01-24T03:22:26Z

src/software/thunderscope/binary_context_managers/runtime_loader.py

+        except (FileNotFoundError, PermissionError, NotADirectoryError):
+            logging.warning(
+                f"Folder for external runtimes {RuntimeManagerConstants.EXTERNAL_RUNTIMES_PATH} could not be accessed."
+            )


We shouldn't log this, instead, we should check if the dir exists, and then create it if its missing. This is shared functionality, refer to #3575 for implementation in the superclass

src/software/thunderscope/binary_context_managers/runtime_loader.py

…time_manager.py to make directory)

adrianchan787 · 2026-01-25T00:08:04Z

Changes done in latest commits (hopefully this helps with review a bit):

Changed default binary name from "localhost" to "Current Fullsystem" (reduces severity of sadness in case someone else calls their AI "localhost")
Removed all caching functionality (fetch name list, read, and write are only called once essentially so no need
Got rid of some safety checks and logging functionality)
Fetch name list now creates the directory if it doesn't exist; read creates the toml file if it doesn't exist
Got rid of the sudo creation of the toml file in setup software

tysm Eric for helping me a lot!

nycrat

👍

Leads approved

adrianchan787 added 2 commits January 17, 2026 10:15

Issue functionality

5940eff

Reverted test framework

30ba4ed

adrianchan787 self-assigned this Jan 17, 2026

GrayHoang reviewed Jan 17, 2026

View reviewed changes

adrianchan787 changed the title ~~Adrian/ai selector backend~~ SIM RFC: AI selector backend Jan 18, 2026

Merge branch 'master' into adrian/ai_selector_backend

4bfda4f

StarrryNight reviewed Jan 19, 2026

View reviewed changes

src/software/thunderscope/thunderscope_main.py Outdated Show resolved Hide resolved

StarrryNight reviewed Jan 19, 2026

View reviewed changes

Andrewyx mentioned this pull request Jan 20, 2026

RuntimeManager Interface #3573

Merged

4 tasks

a-png129 mentioned this pull request Jan 21, 2026

SIM RFC: AI selector interface #3568

Merged

4 tasks

adrianchan787 added 10 commits January 22, 2026 12:25

Added AI CONFIG class

20e020d

Added some comments

d5cd8f5

Merge conflict

e4cf513

Added comment

cacc041

Added spec and reverted bad merge change

e693725

Moved all functionality to runtime loader

72d69e2

Changed all constant names + added AI config functionality

e3cc157

Got rid of a todo

5907687

Added cache, fixed some of the specs

409637c

Removed old constants

ef57804

adrianchan787 marked this pull request as ready for review January 22, 2026 23:17

adrianchan787 requested review from Andrewyx, sauravbanna and williamckha as code owners January 22, 2026 23:17

[pre-commit.ci lite] apply automatic fixes

15b865f

adrianchan787 requested a review from nycrat January 22, 2026 23:36

nycrat reviewed Jan 23, 2026

View reviewed changes

Merge AI interface

86e9f3e

adrianchan787 requested a review from StarrryNight January 23, 2026 18:01

nycrat previously approved these changes Jan 24, 2026

View reviewed changes

Andrewyx reviewed Jan 24, 2026

View reviewed changes

Andrewyx mentioned this pull request Jan 24, 2026

Downloading Binaries #3575

Merged

4 tasks

sauravbanna previously requested changes Jan 24, 2026

View reviewed changes

adrianchan787 added 2 commits January 24, 2026 11:06

Simplified is_valid_runtime

3c725be

Added logging messages for if the retrieved runtime is invalid

7f99351

Andrewyx reviewed Jan 24, 2026

View reviewed changes

adrianchan787 added 3 commits January 24, 2026 12:36

Something something sudo mkdir something (added small function in run…

76a3f5a

…time_manager.py to make directory)

Made config a dataclass

7c3f575

Merge conflicts fixed

5725677

adrianchan787 dismissed nycrat’s stale review via 5725677 January 24, 2026 20:40

pre-commit-ci-lite bot and others added 11 commits January 24, 2026 20:46

[pre-commit.ci lite] apply automatic fixes

5829fc2

Reverted creating directory stuff

c0b61b4

Reverted changes hopefully

2fc927b

Deleted toml creation in setup software sh

7261215

[pre-commit.ci lite] apply automatic fixes

9363f20

Cleaned runtime loader. Will add list of things done in a bit

9968d2e

Merge conflicts

c6f28b1

[pre-commit.ci lite] apply automatic fixes

a39b5d7

Got rid of a space

0c3bdab

Merge conflicts

45c3dc7

Fixed spec

e5abbd7

adrianchan787 force-pushed the adrian/ai_selector_backend branch from 7e5ba76 to e5abbd7 Compare January 25, 2026 00:10

nycrat approved these changes Jan 25, 2026

View reviewed changes

Andrewyx approved these changes Jan 25, 2026

View reviewed changes

adrianchan787 requested a review from sauravbanna January 25, 2026 03:26

nycrat merged commit b6484da into UBC-Thunderbots:master Jan 25, 2026
7 of 13 checks passed

SIM RFC: AI selector backend #3569

SIM RFC: AI selector backend #3569

Uh oh!

Conversation

adrianchan787 commented Jan 17, 2026

Description

Testing Done

Resolved Issues

Length Justification and Key Files to Review

Review Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrianchan787 Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StarrryNight Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StarrryNight commented Jan 19, 2026

Uh oh!

StarrryNight commented Jan 19, 2026

Uh oh!

nycrat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nycrat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lmh-java Jan 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrianchan787 Jan 19, 2026 •

edited

Loading

StarrryNight Jan 19, 2026 •

edited

Loading

Lmh-java Jan 24, 2026 •

edited

Loading