Interactive Path Analysis Feature Integration

[w0lek]

Summary

Added the ability to operate the VPR Viewer in server mode and accept and reply to requests from third-party applications (clients) via socket.

Change log from iteration 1 PR: https://github.com/verilog-to-routing/vtr-verilog-to-routing/pull/2483# which was canceled. This PR is accumulative, it contains all changes from PR 2483#

• Implemented per path element renderer, allowing to render any combination of path and path elements selection.
• Using sockpp wrapper to provide cross-platform communication (previously restricted to UNIX socket only).
• Transitioned to header-based communication protocol instead of delimiter-based to enable sending zipped data.
• Implementing zlib compression of timing report before transmission.

Description

Implemented the ability to run the VPr Viewer in server mode by specifying a few new command-line arguments: --server and --port_num N. Once the server mode is activated, the VPp Viewer waits for a single client connection to be established. Currently there two requests are supported:

1. get n critical path report example: {"CMD":"0","JOB_ID":"1","OPTIONS":"int:path_num:100;string:path_type:setup;string:details_level:netlist;bool:is_flat_routing:0"}

2. hight light critical path elements example: {"CMD":"1","JOB_ID":"3","OPTIONS":"string:path_elements:0#1,2;string:high_light_mode:crit path flylines delays;bool:draw_path_contour:1"}

Motivation and Context

The VPR Viewer server mode enables an extended integration with third-party applications, enhancing the debugging process of FPGA designs. This mode allows parsing reports on the third-party side and brings interactivity to it. Users are able to select paths and path elements freely, and see the result of their selection in the VPR Viewer.

How Has This Been Tested?

The changes are utilized in the Aurora package, where Aurora acts as a client app(in a sense of Interactive Path Analysis Tool), wrapping the execution of the VPR Viewer. Furthermore, these changes have been integrated into FOEDAG already, making them potentially useful for any software component based on FOEDAG.

Aurora v2.6 packages already contains VPR with such functionality.

The package also contains docs/Aurora_Quick_Start_Guide.pdf where in chapter 3.7 there is full feature description with example how to use it.

As long as the command-line argument --server is not specified, the workflow remains unchanged. However, when the --server command-line argument is specified, additional control via socket is enabled, overriding events coming from the UI.

Types of changes

• [ ] Bug fix (change which fixes an issue)
• [x] New feature (change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• [x] My change requires a change to the documentation
• [ will be done in separate PR] I have updated the documentation accordingly
• [x] I have added tests to cover my changes
• [x] All new and existing tests passed

[w0lek]

Unresolved tasks from https://github.com/verilog-to-routing/vtr-verilog-to-routing/pull/2483

1. Add end user documentation (document the command line option, and this feature in the graphics section (probably a new tab).
2. We should have Doxygen comments on the server etc. code (which I think you already have) and add the resulting hyperlinked documentation to read the docs, probably under VPR API. There is guidance on how to do that in the VTR Developer Guide (it's a short amount of code to write once you have the Doxygen comments, and the Guide tells you where to write it).

[vaughnbetz]

@soheilshahrouz can you take a look at this one when it is ready?

[soheilshahrouz]

@soheilshahrouz can you take a look at this one when it is ready?

I can review it over the weekend.

[soheilshahrouz]

@w0lek Is this PR ready for review or you have more commits to push?

[w0lek]

@soheilshahrouz Up to now i have two failures in CI: 1) Python Lint and 2) Check Compilation Warnings. But i am not sure if failure caused by mine changes or not, and how they are critical. And still the end user documentation is missing. I am not able to build doc target yet, in order to check that my doc changes in *.rst gives consistent result. when i run make html from the doc folder i got "ModuleNotFoundError: No module named 'markdown_code_symlinks'".

And .github/scripts/hostsetup.sh didn't finish successfully on my local build node ( i tried ubuntu 20.04 and ubuntu 22.04). This is what i am still investigating right now.

If these 3 active issues are not a blocker for a reviewing this PR, the review could be started. I also may push end user doc changes into separate branch, in order to not change the branch which currently in PR review. If that possible, to add end user documentation via other branch a bit later, it would be nice.

[vaughnbetz]

Yes, we can put the documentation in a separate PR if that's easier.

[vaughnbetz]

We are generally python lint clean and warning clean at least in vpr, so those should be fixed before merging so others don't suddenly see CI failures. Usually they're pretty easy to fix after you examine the detailed logs from those tests, which will show the warnings and/or lint issues.

[w0lek]

@soheilshahrouz i moved out PR from a draft. The user end docu will be made in separate PR. I fixed CI warning tests and Python Lint. However the Python Lint still fails me on my local fork, complaining to the vpr/thirdparty/sockpp/conanfile.py which i deleted here https://github.com/verilog-to-routing/vtr-verilog-to-routing/pull/2526/commits/087fb13aedf1c25923824dccc0ab0f6857f35ba3 . So i am not sure why this action fails. May you please trigger CI actions?

[vaughnbetz]

It looks like everything passed. I'll just wait a little while to give Soheil time to review. I also sent an invitation for you to join the vtr project so your jobs launch automatically etc.

[AlexandreSinger]

Thank you for the PR! I also noticed that there are some warnings introduced by this PR. I have been working on trying to remove all of the warnings. Could you please resolve them. They can be found in the following CI run at line 701: https://github.com/verilog-to-routing/vtr-verilog-to-routing/actions/runs/8583394130/job/23537150300?pr=2526#step:6:701

Most of them appear to be missing include directories and casts that discard qualifiers.

[w0lek]

Thank you for the PR! I also noticed that there are some warnings introduced by this PR. I have been working on trying to remove all of the warnings. Could you please resolve them. They can be found in the following CI run at line 701: https://github.com/verilog-to-routing/vtr-verilog-to-routing/actions/runs/8583394130/job/23537150300?pr=2526#step:6:701

Most of them appear to be missing include directories and casts that discard qualifiers.

Thank you for a review! Yes i will do this after i move the sockpp to libs/EXTERNAL/ as a git submodule.

[w0lek]

TODO: use snakecases for variable names, function names and method names. "{" must be on the same line as ")" in function definition.

[w0lek]

TODO: use snakecases for variable names, function names and method names. "{" must be on the same line as ")" in function definition.

this is done

[w0lek]

@AlexandreSinger to fix warning compilation in sockpp thirdparty library, i created fork with custom branch where the warnings were fixed, and set using this fork instead of upstream in vpr as git submodule. I tested locally, let's see what CI will show(maybe some more warnings needs to be fixed). I have no time to properly contribute sockpp fixes from fork to sockpp upstream right now, but could do it later. Hope this approach (of using my fork of sockpp together with) is fine as temporary solution. Please let me know if it's ok for you.

Note: Also, integration new thridparty library as a replacement for sockpp, if we will select that way, might take some time, since it will require the code changes in gateio and long period of stability testing phase (small and big HW designs). sockpp library were already tested and proved it works stable with transmitting big amount of data, so we would like to stick with it right now.

Please let me know what do u think.

https://github.com/verilog-to-routing/vtr-verilog-to-routing/pull/2526/commits/4ada45c9b50a1533b2ba0d3a2baadc49d5adb9ad

[w0lek]

@soheilshahrouz thank you for reviewing PR. i have finished all review tasks, until there will no be new one found. In short sumup of some relevant changes:

• all new code now uses snakecase to follow common style
• the server mode could be controlled by cmake variable VPR_USE_SERVER.
• by default VPR_USE_SERVER is on, because i wanted to make sure that CI pass all new cases. Alternative way would be off it by default but having new CI stage to build vpr with explicit VPR_USE_SERVER=on.
• the compile time warnings on thirdparty library sockpp are fixed, sockpp now is a git submodule.
• [integration regression test passed] after all changes were made with code in vpr repository, the upstream vpr binary were tested along with modificated Aurora package, where spot test of Interactive Path Analysis were made.
• the end user doc will be in separate PR, later

@soheilshahrouz please let me know if i need do something else about this PR?

[AlexandreSinger]

@w0lek This was mentioned in the VTR weekly Industry Sync-up. Thank you for clarifying that this code is enabled by default, and disabled automatically when graphics are disabled. As @vaughnbetz mentioned, since it is possible for graphics to be on and this feature to be off, we should still test to ensure that turning off your feature does not cause any issues in CI.

A CI test for this can be added simply by adding a new regression test. This requires just adding a new test in the following list: https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/1a103beba370d0a8e6c241d95796fad5438c149c/.github/workflows/test.yml#L184-L230

This list is a list of regression tests that are run with different build options enabled / disabled. It should be fairly straightforward to add a new test for when your feature is disabled. This file can be updated in a commit on your PR and the CI can then be rerun when you push these changes.

[w0lek]

@AlexandreSinger thank you for detailed explanation, i have added test case when GRAPHICS=on but SERVER=off https://github.com/verilog-to-routing/vtr-verilog-to-routing/pull/2526/commits/761c1be7cfe2d9518e47c9b060c0c6e9737668ed

Also FYI:

1. the end user doc PR is ready to be created, as soon as this PR will be on master, i am going create end user doc PR. Right now i create PR only on fork for personal review https://github.com/w0lek/vtr-verilog-to-routing/pull/9
2. creating simple IPA client app was approved by team, i am going to create PR with that in a week. Note: This was asked by @vaughnbetz on last VTR meeting.
3. using boost.asio instead of sockpp is scheduled for future (this proposal was accepted by team). But that would require some time to implement and test.

[AlexandreSinger]

@w0lek Thank you for adding the CI test, it looks like it ran successfully! Reviewing the CI logs, it looks like it exposed a couple very small warnings (unused parameters) in vpr/src/draw/draw_basic.cpp:

These warnings come from this function: https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/0a0d009f837393c309203aa42b41ab760f3fae00/vpr/src/draw/draw_basic.cpp#L1112-L1118

I think if you just cast these arguments to void when NO_SERVER is defined it should remove the warnings (just to explicitly mark that this function will do nothing with those variables when the server is disabled).

However, maybe consider throwing an error when this function is called without the server being enabled; or just removing the declaration and header entirely when the server is not defined.

[w0lek]

@AlexandreSinger thank you for the catch! I made draw_crit_path_elements call more generic, now it doesn't depend on the Server module availability. So warnings with unused variables should be gone now. Please let me know if my solution not ok. Fixed here: https://github.com/verilog-to-routing/vtr-verilog-to-routing/pull/2526/commits/5efd5c187bfdd783ce159b62ceb452a4aa5a9590

[vaughnbetz]

Will merge when CI is green. A last few suggestions (which look like good ones) from @soheilshahrouz in case you can get those in too.

[w0lek]

Thanks for applying the previous comments.

I added a few comments to suggest minor improvements.

thank you, i am going to fix them, on Friday(tomorrow) or on this weekend as latest option

[w0lek]

@vaughnbetz @soheilshahrouz @AlexandreSinger Thank you for a review and feedback, i have corrected all moments. Please let me know if i need to correct something else.

[vaughnbetz]

Thank you @w0lek ! And thanks to @soheilshahrouz and @AlexandreSinger for the detailed reviews. A long awaited merge ... :).