Bugfix/760 Fixes for debugger

sassy-asjp commented 3 months ago

Tickets addressed: Closes #760 Closes #249
Review: By commit
Merge strategy: Merge (no squash)

Description

PythonVariableLogger was modified to create properties in __new__ instead of using __getattr__. The original snippet on #760 does not work because properties must be set on the class object not the object itself. A new solution was written. Thanks @juan-g-bonilla
C Message Recorders were modified to be created from the C/C++ side similar to C++ Message Recorders, rather than from the Python side.

Verification

The example script from #760 was run with all unproblematic lines uncommented to confirm that the weird debugger behavior was not present. It seems prohibitively difficult unit test this bug or fix.

Unit tests were run to check for regressions.

Documentation

None

Future work

Follow up with the SWIG project if we can demonstrate the weird behavior without involving the rest of Basilisk. This would allow us to undo the fix for at least pythonVariableLogger so the custom error message in __getattr__ can be used.

sassy-asjp commented 3 months ago

@schaubh @juan-g-bonilla

My initial approach for pythonVariableLogger ended up not working as expected, but I wrote a new one, and this should be ready for review now

schaubh commented 3 months ago

Howdy @sassy-asjp , thanks for the contribution. I'll take a look at this soon. Got several other PRs to wrap up this week. @juan-g-bonilla , thanks for your thoughts on this PR as it modifies code you wrote. I added you as a reviewer to leave comments.

juan-g-bonilla commented 3 months ago

@sassy-asjp Thanks for this PR and sorry I missed the fact that properties should be class variables on our initial discussion. I’m camping without access to a laptop, so bear with me if the code I suggest doesn’t work out of the box. I think changing the class attribute is a bit (too) hacky. I’d feel more comfortable if instead we created the dynamic class on new and created the instance from the beginning with this dynamic type:


class PythonVariableLogger:

    def __new__(cls, logging_functions, min_log_period):
        properties = {
            name: property(
                    lambda self, name=name: np.array(self._variables[name])
                ) 
                for name in logging_functions
        }
        dyncls = type('PythonVariableLoggerDynamic', (cls, ), properties)
        return super(PythonVariableLogger, dyncls).__new__(dyncls)

sassy-asjp commented 3 months ago

@juan-g-bonilla

Thanks for the change! I had to modify it to include the min_log_period default value but it generally works.

CI failures seem unrelated.

/home/runner/.conan/data/protobuf/3.17.1/_/_/package/2dbf65f76c0469903ce48756c39d50cd4e721678/bin/protoc: /lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.29' not found (required by /home/runner/.conan/data/protobuf/3.17.1/_/_/package/2dbf65f76c0469903ce48756c39d50cd4e721678/bin/protoc)

sassy-asjp commented 3 months ago

@schaubh

It seems like the protobuf/3.17.1 in the conan cache is only compatible with Ubuntu 22.04? runner.os is just Linux so I suspect some binaries built on Ubuntu 22.04 that were saved in the conan cache then trying to reuse them on Ubuntu 20.04 causes problems. This probably was not caught when initially merged as the cache had not been created yet.

The likely solution is to add the OS version to the cache key. I added the below to my branch and it passes CI. Let me know if you actually want to make the change elsewhere though.

diff --git a/.github/workflows/pull-request.yml b/.github/workflows/pull-request.yml
index 4602c000c..9f0dcf8fb 100644
--- a/.github/workflows/pull-request.yml
+++ b/.github/workflows/pull-request.yml
@@ -41,7 +41,7 @@ jobs:
         id: cache-conan
         uses: actions/cache@v4
         env:
-          cache-name: cache-conan-packages
+          cache-name: cache-conan-packages-ubuntu-20-04
         with:
           # conan cache files are stored in `~/.conan` on Linux/macOS
           path: ~/.conan
@@ -105,7 +105,7 @@ jobs:
         id: cache-conan
         uses: actions/cache@v4
         env:
-          cache-name: cache-conan-packages
+          cache-name: cache-conan-packages-ubuntu-20-04
         with:
           # conan cache files are stored in `~/.conan` on Linux/macOS
           path: ~/.conan
@@ -169,7 +169,7 @@ jobs:
         id: cache-conan
         uses: actions/cache@v4
         env:
-          cache-name: cache-conan-packages
+          cache-name: cache-conan-packages-ubuntu-22-04
         with:
           # conan cache files are stored in `~/.conan` on Linux/macOS
           path: ~/.conan
@@ -234,7 +234,7 @@ jobs:
         id: cache-conan
         uses: actions/cache@v4
         env:
-          cache-name: cache-conan-packages
+          cache-name: cache-conan-packages-ubuntu-22-04
         with:
           # conan cache files are stored in `~/.conan` on Linux/macOS
           path: ~/.conan
@@ -298,7 +298,7 @@ jobs:
         id: cache-conan
         uses: actions/cache@v4
         env:
-          cache-name: cache-conan-packages
+          cache-name: cache-conan-packages-ubuntu-22-04
         with:
           # conan cache files are stored in `~/.conan` on Linux/macOS
           path: ~/.conan

schaubh commented 3 months ago

Howdy @sassy-asjp , thanks for the fix on the latest CI builds to use unique Linux cache names. I created a quick PR #770 with just this fix to help other on-going PRs. This will give us time to properly test your debugger solution and other PRs are not held up by this issue.

schaubh commented 2 months ago

@juan-g-bonilla , welcome back from hiking! Before I close this, any final thoughts on this branch?

I pulled the branch and did a clean build without issues on latest macOS. All tests ran fine.

schaubh commented 2 months ago

@sassy-asjp , this PR is ready to go. I just need it to be rebased on latest develop. I don't have write access to this branch so I can't do this on my end.

AVSLab / basilisk