trying to reduce the boards in the report

2bndy5 commented 2 years ago

I just discovered this great action, so I thought I'd try it out on an open PR (during sync event). At first it worked well, but the workflow tests so many boards that (I think) it maxed out the allowed REST API payload (data was missing from resulting thread comment's full report table). So, I thought I'd try limiting the reports to just arduino:avr:nano, arduino:samd:mkrzero, and ATTinyCore:avr:attinyx5 boards....

The options modified for arduino/compile-sketches

# for job that tests all examples
          enable-deltas-report: ${{ matrix.fqbn == 'arduino:avr:nano' || matrix.fqbn == 'arduino:samd:mkrzero' }}
# for job that tests only ATTiny compatible examples
          enable-deltas-report: ${{ matrix.fqbn == 'ATTinyCore:avr:attinyx5' }}

But I get an error saying:

Traceback (most recent call last):
  File "/reportsizedeltas/reportsizedeltas.py", line 737, in <module>
    main()  # pragma: no cover
  File "/reportsizedeltas/reportsizedeltas.py", line 32, in main
    report_size_deltas.report_size_deltas()
  File "/reportsizedeltas/reportsizedeltas.py", line 91, in report_size_deltas
    self.report_size_deltas_from_local_reports()
  File "/reportsizedeltas/reportsizedeltas.py", line 100, in report_size_deltas_from_local_reports
    sketches_reports = self.get_sketches_reports(artifact_folder_object=sketches_reports_folder)
  File "/reportsizedeltas/reportsizedeltas.py", line 295, in get_sketches_reports
    not in report_data[self.ReportKeys.boards][0][self.ReportKeys.sizes][0])
KeyError: 'sizes'

Here's the complete workflow run. Here's the complete workflow yaml.

I'll keep digging, but I thought I'd raise this here because I'm not completely familiar with this action's src script.

ps - Initially, I thought this action would be written in JS (using actions/toolkit), so there wouldn't be any need to pull a docker img just to have an isolated python env (which adds ~30s build time on each run). But, that's an issue for another time like when I know more about how this action is executed.

per1234 commented 2 years ago

Hi @2bndy5. Thanks for your interest in this action.

The cause of the error is these conditionals:

When those evaluate to false, the deltas are not calculated by the arduino/compile-sketches action, and so this data expected by the arduino/report-size-deltas action is missing from some of the sketches reports in the workflow artifact.

The simple solution is to not upload sketches reports when you are setting the enable-deltas-report input to false. One approach for doing that is demonstrated here:

diff --git a/.github/workflows/build_arduino.yml b/.github/workflows/build_arduino.yml
index 8b6c1c8a..b355e629 100644
--- a/.github/workflows/build_arduino.yml
+++ b/.github/workflows/build_arduino.yml
@@ -90,7 +90,16 @@ jobs:
           - "arduino:megaavr:uno2018"
           # - "arduino:megaavr:nano4809"  # board not found
           - "arduino:sam:arduino_due_x_dbg"
-
+        # By default, don't generate size deltas data.
+        enable-deltas-report:
+          - false
+        # Make board type-specific customizations to the matrix jobs
+        include:
+          - fqbn: arduino:avr:nano
+            # Generate size deltas data for this board
+            enable-deltas-report: true
+          - fqbn: arduino:samd:mkrzero
+            enable-deltas-report: true

     steps:
       - name: Checkout
@@ -118,11 +127,13 @@ jobs:
           #   - examples/old_backups/recipes/nordic_fob
           #   - examples/old_backups/recipes/pingpair_maple
           fqbn: ${{ matrix.fqbn }}
-          enable-deltas-report: ${{ matrix.fqbn == 'arduino:avr:nano' || matrix.fqbn == 'arduino:samd:mkrzero' }}
+          enable-deltas-report: ${{ matrix.enable-deltas-report }}
           sketches-report-path: ${{ env.SKETCHES_REPORTS }}

       # This step is needed to pass the size data to the report job 
       - name: Upload sketches report to workflow artifact
+        # Only upload sketches reports with deltas data to the workflow artifact that will be consumed by the report job
+        if: ${{ matrix.enable-deltas-report }}
         uses: actions/upload-artifact@v2
         with:
           name: ${{ env.SKETCHES_REPORTS }}
@@ -159,6 +170,11 @@ jobs:
           - ATTinyCore:avr:attiny1634
           - ATTinyCore:avr:attiny1634opti
           - ATTinyCore:avr:attinyx313
+        enable-deltas-report:
+          - false
+        include:
+          - fqbn: ATTinyCore:avr:attinyx5
+            enable-deltas-report: true

     steps:
       - name: Checkout
@@ -175,11 +191,12 @@ jobs:
             - examples/rf24_ATTiny/rf24ping85
             - examples/rf24_ATTiny/timingSearch3pin
           fqbn: ${{ matrix.fqbn }}
-          enable-deltas-report: ${{ matrix.fqbn == 'ATTinyCore:avr:attinyx5' }}
+          enable-deltas-report: ${{ matrix.enable-deltas-report }}
           sketches-report-path: ${{ env.SKETCHES_REPORTS }}

       # This step is needed to pass the size data to the report job 
       - name: Upload sketches report to workflow artifact
+        if: ${{ matrix.enable-deltas-report }}
         uses: actions/upload-artifact@v2
         with:
           name: ${{ env.SKETCHES_REPORTS }}

This assumes you have no intent to use those other reports. If you do need them, you can configure the workflow so that one workflow artifact with the deltas data is created for consumption by the arduino/report-size-deltas action, and another workflow artifact created for all the jobs.

I understand that this experience indicates two areas for improvement in the action:

Clear communication of what went wrong when the sketches report has an invalid data format.
Ability to handle sketches reports that are missing the data.

The first is indisputable.

As for the second, I'm a bit ambivalent. I'm not sure whether or not it makes sense to support being fed data that was not intended for the action's consumption because this might mask problems with the user's workflow. In your case, it would have meant the action processing a large number of reports unnecessarily.

the workflow tests so many boards that (I think) it maxed out the allowed REST API payload (data was missing from resulting thread comment's full report table).

The action is intended to avoid exceeding the maximum GitHub comment length limit: https://github.com/arduino/report-size-deltas/pull/11

The arduino/ArduinoCore-samd repo's workflow compiles a very large number of sketches, which caused it to exceed this. The solution was to drop the full report data from the comment when it would have caused it to go over. You can see an example of that here: https://github.com/arduino/ArduinoCore-samd/pull/659#issuecomment-1013784842

2bndy5 commented 2 years ago

I thought it might have been my approach. I'll try your suggestions and report back.

2bndy5 commented 2 years ago

Yep that approach worked better than my initial (half-cocked) attempt.

Clear communication of what went wrong when the sketches report has an invalid data format.

Looking at the src, I see that the following message would've been helpful enough: https://github.com/arduino/report-size-deltas/blob/1aebe8643b99600f542637796da6f5555422c843/reportsizedeltas/reportsizedeltas.py#L308-L309

But, the trace back message didn't get to that line because this condition wasn't coded for KeyError type exceptions: https://github.com/arduino/report-size-deltas/blob/1aebe8643b99600f542637796da6f5555422c843/reportsizedeltas/reportsizedeltas.py#L292-L296

I hope that helps. I'm not confident enough to suggest a solution for better error prompts from this action's src. I'll close this issue now that my mistakes were amended (thanks again @per1234 ), and better error prompts seems like a worthy new issue (albeit probably specific to my mistake).

arduino / report-size-deltas

trying to reduce the boards in the report #19