Skip to content

Throw an exception if the user tries to render a trace without stepping the simulation #472

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: development
Choose a base branch
from
Open

Throw an exception if the user tries to render a trace without stepping the simulation #472

wants to merge 1 commit into from

Conversation

gaborszita
Copy link
Contributor

When the user tries to render a trace without stepping the simulation, PyRTL gives this weird error:

gabi@Gabors-MacBook-Pro PyRTL % python3 test.py
Traceback (most recent call last):
  File "/Users/gabi/Documents/PyRTL/test.py", line 8, in <module>
    sim.tracer.render_trace()
    ~~~~~~~~~~~~~~~~~~~~~~~^^
  File "/Users/gabi/Documents/PyRTL/pyrtl/simulation.py", line 1787, in render_trace
    self.render_trace_to_text(
    ~~~~~~~~~~~~~~~~~~~~~~~~~^
        trace_list=trace_list,
        ^^^^^^^^^^^^^^^^^^^^^^
    ...<5 lines>...
        segment_size=segment_size,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/Users/gabi/Documents/PyRTL/pyrtl/simulation.py", line 1876, in render_trace_to_text
    current_symbol_len = max(
        len(
    ...<4 lines>...
        for v in trace
    )
ValueError: max() iterable argument is empty

test.py:

import pyrtl

a = pyrtl.Register(name="test_reg", bitwidth=3)
a.next <<= a+1

sim = pyrtl.Simulation()

sim.tracer.render_trace()

Output after this PR:

gabi@Gabors-MacBook-Pro PyRTL % python3 test.py
Traceback (most recent call last):
  File "/Users/gabi/Documents/PyRTL/test.py", line 8, in <module>
    sim.tracer.render_trace()
    ~~~~~~~~~~~~~~~~~~~~~~~^^
  File "/Users/gabi/Documents/PyRTL/pyrtl/simulation.py", line 1760, in render_trace
    raise PyrtlError("You need to step the simulation at least once to render a trace.")
pyrtl.pyrtlexceptions.PyrtlError: You need to step the simulation at least once to render a trace.

I was working on pyrtlnet and it took me around 10 minutes to figure out why I'm getting this error so I decided to open this little PR 😅

@gaborszita
Copy link
Contributor Author

Seems a CompileSim test is failing, here's why: render_trace_to_text throws an exception if we are not tracing any signals and a there is a check for this exception in the CompileSim tests. However, __len__ does the same check, but it just throws the exception with a different message, and since with this change __len__ is now called before render_trace_to_text, __len__'s exception is thrown:

image

The two error messages are basically the same thing just described differently, so there is no logic error in the code. There are multiple ways to solve this problem, the simplest is just to copy the error message from render_trace_to_text to __len__. @fdxmw what do you suggest we do?

@fdxmw
Copy link
Member

fdxmw commented Sep 8, 2025

Thank you for improving this!

The two error messages are basically the same thing just described differently, so there is no logic error in the code. There are multiple ways to solve this problem, the simplest is just to copy the error message from render_trace_to_text to len. @fdxmw what do you suggest we do?

This is why I'm generally not a fan of tests that check error messages :) These tests are too sensitive, and often fail when the code's behavior is still correct. I'd be okay with just removing the assertEqual that checks the error message; I think it's sufficient to just check that PyrtlError was thrown.

Please also run ruff check and ruff format on your change. Some minor readability improvements will be needed before we can merge it. In the future, please run tox to test your changes before you open a pull request; tox will also run ruff for you.

In particular, try not to rely on GitHub's pull request actions to run tests for you, because opening a pull request triggers notifications for people watching the repository, which creates unnecessary noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gaborszita @fdxmw