Changes to enable discrete actions > 2

[Ivan-267]

Experimental changes to enable discrete actions > 2 using SB3 (during training and with exported onnx). This also requires the changes in the main repository to work: https://github.com/edbeeching/godot_rl_agents/pull/121

[yaelatletl]

Hey, I saw your comments in the linked PR and noticed you changed how the model is loaded. I went ahead and checked the code, the function you just changed the loading of the inference session is a function expected to be called around 30 to even 120 times a second. Does the current method interfere with your PR's functionality? I could help you sort that out, but you cannot have the load in the same function as the inference run.

[Ivan-267]

@yaelatletl thanks for your review and this importance notice, I will check the code for this and respond with more detail and/or alterations.

[yaelatletl]

Thanks, also, you should really load the model once and make it accessible for the object so you can run as many inferences as you want from the same InferenceSession

[Ivan-267]

@yaelatletl Thanks for spotting this. This was an oversight on my part. The point was to handle the disposal of disposable resources, but that can also be done with e.g. .dispose() after the session is over, but we will need to know at what point it is over. I will alter the implementation.

[yaelatletl]

I understand the concern about resource disposal, it is an oversight on my part that you just made me notice. The way this is set up creates at least two orphan nodes that won't be freed at the time of closing the project, I'll fix that on the gd script side of things.

[Ivan-267]

In one approach, Godot could signal when the session is over by calling a method that takes care of the disposal.

Also I forgot to add about the file loading method, since onnxruntime can load onnx files, I simplified the loading so that we don't need to depend on Godot's file access to load the file. I'm not sure if this can cause any regressions. Do you know if there is some advantage to using GD file access instead?

Regarding these lines: https://github.com/edbeeching/godot_rl_agents_plugin/blob/fc67722fc780f10f9ca1b3f259185f4e12b5b423/addons/godot_rl_agents/onnx/csharp/ONNXInference.cs#L56-L57

I didn't dispose of them specifically since it's possible that disposing results disposes of those values. While I'm not 100% certain, to test that I tried disposing results and then accessing output1 afterwards, which resulted in an error message that the value is disposed.

[yaelatletl]

In the Godot side of things, even though the wrapper is a node, it's actually being used as a resource or object, so I have to use notifications instead. Using the Godot loader is better, as it ensures compatibility across platforms and that the resources can be accessed after a game is exported. Yes, both outputs are objects, you could dispose of them at the end of the function, just before the return.

[Ivan-267]

Perhaps you could implement the needed changes on the gdscript side first, since changes on both sides (gdscript, c#) will need to be synchronized.

Using` the Godot loader is better, as it ensures compatibility across platforms and that the resources can be accessed after a game is exported.

I agree with that and I will return your method, hopefully the built-in loading method should work on all supported platforms as well, but your method will ensure compatibility and make sure that e.g. parsing paths is done the same way as in Godot. When it comes to loading an .onnx model in an exported game, for that we currently need to move the onnx file to the same location as the exported .exe (on Windows) in either case (if the specified onnx URL is just the filename), it's not currently getting copied and used automatically as a part of the game as far as I'm aware.

Yes, both outputs are objects, you could dispose of them at the end of the function, just before the return.

They can also be disposed with using (that should call .dispose() before the method return automatically), but the data may already be getting disposed when results itself is disposed, leaving only the references to data that was disposed, which should be disposed automatically by the garbage collector at some point. In that case (if that is correct), additionally calling dispose specifically for result1 and result2 might not be necessary.

There is also a note in the docs (https://tomwildenhain-microsoft.github.io/onnxruntime/docs/get-started/with-csharp.html):

Multiple inference runs with fixed sized input(s) and output(s) If the model have fixed sized inputs and outputs of numeric tensors, you can use “FixedBufferOnnxValue” to accelerate the inference speed. By using “FixedBufferOnnxValue”, the container objects only need to be allocated/disposed one time during multiple InferenceSession.Run() calls. This avoids some overhead which may be beneficial for smaller models where the time is noticeable in the overall running time.

We could also check if we can use that and how it affects inference performance as a potential future change.

Edit: Here is one possible option I could implement on the C# sharp side with your model loading method returned, where DisposeSession() would need to be called from Godot when inference is done. After that, running inference again (without initializing again) would cause an error.

        private InferenceSession session;

        /// <summary>
        /// Path to the ONNX model. Use Initialize to change it. 
        /// </summary>
        private string modelPath;
        private int batchSize;

        private SessionOptions SessionOpt;

        //init function
        /// <include file='docs/ONNXInference.xml' path='docs/members[@name="ONNXInference"]/Initialize/*'/>
        public void Initialize(string Path, int BatchSize)
        {
            modelPath = Path;
            batchSize = BatchSize;
            SessionOpt = SessionConfigurator.GetSessionOptions();
            session = LoadModel(modelPath);
        }

        /// <summary>
        /// Disposes of the session. Called when inference is no longer needed.
        /// RunInference should not be called after the session is disposed.
        /// </summary>
        public void DisposeSession()
        {
            session.Dispose();
        }

Just in case .Dispose() does more than .Close() in C# I added a using statement as in the docs (https://docs.godotengine.org/en/stable/classes/class_fileaccess.html#class-fileaccess-method-close)

In C# the reference must be disposed manually, which can be done with the using statement or by calling the Dispose method directly.

        /// <include file='docs/ONNXInference.xml' path='docs/members[@name="ONNXInference"]/Load/*'/>
        public InferenceSession LoadModel(string Path)
        {
            using Godot.FileAccess file = FileAccess.Open(Path, Godot.FileAccess.ModeFlags.Read);
            byte[] model = file.GetBuffer((int)file.GetLength());
            return new InferenceSession(model, SessionOpt); //Load the model
        }

[Ivan-267]

Superseded by https://github.com/edbeeching/godot_rl_agents_plugin/pull/40.

The branch can still be kept for reference since this implementation has an advantage, it sends only action values for discrete actions (not logits), so it could be more efficient with large spaces.