Skip to content

Allow dumping Python wrapper even when the shared lib is cached. #288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: pymtl4.0-dev
Choose a base branch
from

Conversation

ptpan
Copy link
Contributor

@ptpan ptpan commented Feb 20, 2025

Fixes a bug where Verilator VCD dumps overwrite each other even when the test names are different. The bug was caused by skipping Python wrapper dumping when the shared library is cached, which is incorrect because certain parts of the py wrapper may change (such as the Verilator VCD filename) even when the library is cached.

The solution is to dump the Python wrapper (guarded by a lock among processes for pytest-xdist) when such situations occur.

@ptpan ptpan requested review from cbatten and yo96 February 20, 2025 05:26
@cbatten
Copy link
Contributor

cbatten commented Feb 21, 2025

Hmmm ... this doesn't seem to fix the bug with the example shown below. Did this example work for you? I did some debugging and if you look below I can can see that the PyMTL wrapper is updated. It now has "large" embedded in it ... but if I add some trace debugging to the verilator_wrapper_py_template.py to print out verilator_vcd_file it is not getting updated which I think means reloading the module here might not be working? I can tell that reload is getting callled but it doesn't seem to actually be updating the in memory representation? Thoughts?

    if wrapper in sys.modules:
      # Reload the wrapper module in case the user has updated the wrapper
      reload(sys.modules[wrapper])
    else:
      # importlib.import_module inserts the wrapper module into sys.modules
      importlib.import_module(wrapper)
 % mkdir -p pymtl3-vcd-test
 % cd pymtl3-vcd-test
 % python3 -m venv pymtl3
 % source pymtl3/bin/activate
 % pip install git+https://github.com./cornell-brg/pymtl3@pp482-verilator-vcd-bug-fix  
 % git clone [email protected]:cornell-ece6745/ece6745-tut3-verilog
 % cd ece6745-tut3-verilog
 % TOPDIR=$PWD

 % cd $TOPDIR/sim/tut3_verilog/regincr
 % cat > RegIncr.v \
<<'END'
`ifndef TUT3_VERILOG_REGINCR_REG_INCR_V
`define TUT3_VERILOG_REGINCR_REG_INCR_V

module tut3_verilog_regincr_RegIncr
(
  input  logic       clk,
  input  logic       reset,
  input  logic [7:0] in_,
  output logic [7:0] out
);

  logic [7:0] reg_out;
  always @( posedge clk ) begin
    if ( reset )
      reg_out <= 0;
    else
      reg_out <= in_;
  end

  assign out = reg_out + 1;

endmodule

`endif /* TUT3_VERILOG_REGINCR_REG_INCR_V */
END

 % mkdir -p $TOPDIR/sim/build
 % cd $TOPDIR/sim/build
 % pytest ../tut3_verilog/regincr/test/RegIncr_test.py -v --dump-vcd
 % ls *.vcd
 tut3_verilog.regincr.test.RegIncr_test__test_large.vcd
 tut3_verilog.regincr.test.RegIncr_test__test_small.vcd
 tut3_verilog.regincr.test.RegIncr_test__test_small_top.verilator1.vcd

% grep verilator_vcd_file RegIncr_noparam_v.py
        verilator_vcd_file = "tut3_verilog.regincr.test.RegIncr_test__test_large_top.verilator1.vcd"

@ptpan
Copy link
Contributor Author

ptpan commented Feb 21, 2025

I tested on my laptop with Python 3.8.5 on this specific case, but I am able to get the extra verilator .VCD file:

$ [venv] python3 --version
Python 3.8.5
$ [venv] pytest ../tut3_verilog/regincr/test/RegIncr_test.py -v --dump-vcd

=========================== test session starts ===========================
platform darwin -- Python 3.8.5, pytest-8.3.4, pluggy-1.5.0 -- cornell-ece6745-vcd-debug/venv/bin/python3
cachedir: .pytest_cache
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase(PosixPath('cornell-ece6745-vcd-debug/ece6745-tut3-verilog/sim/build/.hypothesis/examples'))
rootdir: cornell-ece6745-vcd-debug/ece6745-tut3-verilog/sim
configfile: pytest.ini
plugins: hypothesis-6.113.0, xdist-3.6.1, pymtl3-3.1.16
collected 2 items

../tut3_verilog/regincr/test/RegIncr_test.py::test_small PASSED     [ 50%]
../tut3_verilog/regincr/test/RegIncr_test.py::test_large PASSED     [100%]

============================ 2 passed in 6.14s ============================
$ [venv] ls *.vcd        [19:45:07]
tut3_verilog.regincr.test.RegIncr_test__test_large.vcd
tut3_verilog.regincr.test.RegIncr_test__test_large_top.verilator1.vcd
tut3_verilog.regincr.test.RegIncr_test__test_small.vcd
tut3_verilog.regincr.test.RegIncr_test__test_small_top.verilator1.vcd

I suspect the sys.modules are not reloaded correctly when the new Python wrapper gets dumped, but not sure if this is because of Python version or something else.

@cbatten
Copy link
Contributor

cbatten commented Feb 21, 2025

Hmmm ... we are using Python 3.11.9 ... do you think that might be the issue?

@cbatten
Copy link
Contributor

cbatten commented Feb 21, 2025

Ah ha! It does work if I just use --dump-vcd alone but it does not work if I use --test-verilog and --dump-vcd ... thoughts?

@cbatten
Copy link
Contributor

cbatten commented Feb 27, 2025

I could not get this to reliably work. Most of the time it would not dump both VCD files but every once and a while it would. I did a bunch of debugging and I really think this has something to do with the shared library stuff. I came up with a different approach:

  • verilator_vcd_file = ""
    if int(s._ip_cfg.vl_trace):
    if bool(s._ip_cfg.vl_trace_filename):
    verilator_vcd_file = f"{{s._ip_cfg.vl_trace_filename}}.verilator1.vcd"
    else:
    verilator_vcd_file = "{component_name}.verilator1.vcd"

    All I did was change the Python wrapper so it instead of hard coding the VCD file name into the wrapper we just get it from the metadata. This seems to work?
    @ptpan can you take a look. Do you think there are any issues with this approach?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants