Skip to content
This repository was archived by the owner on Mar 25, 2025. It is now read-only.

Add debug logging to parser#1229

Merged
pramodk merged 4 commits into
masterfrom
jelic/add_logging_parser
Jun 17, 2024
Merged

Add debug logging to parser#1229
pramodk merged 4 commits into
masterfrom
jelic/add_logging_parser

Conversation

@JCGoran

@JCGoran JCGoran commented Mar 21, 2024

Copy link
Copy Markdown
Contributor

In case of issues like #1217, we get a very short log trace like the below:

$ nmodl invalid_name.mod
libc++abi: terminating due to uncaught exception of type std::runtime_error: NMODL Parser Error : syntax error, unexpected ., expecting NAME [Location : 6.11]
    LOCAL _kf
----------^

[1]    40873 abort      nmodl invalid_name.mod

With this change, we can run with --verbose trace to get more information what went wrong:

$ nmodl --verbose trace invalid_name.mod

See trace.log for the full output of the parser.

Of course, --verbose trace will also work if we have a valid file as well.

Some implementation notes:

  • I would've added the logs directly to the logger class, but unfortunately spdlog removed the ability to use streams a while back, while Bison requires exactly a stream-like object to dump its logs. Option 2 was rewriting our own logger to add the stream there, but this would require changing all calls to logger->[LEVEL] throughout the codebase because our custom wrapper class isn't actually used, so I opted for adding it to the driver instead to keep things simple
  • initially I wanted to add logs for Flex as well, but it turns out that it stubbornly dumps everything to std::cerr. I tried to work my way around it, but ended up with segfaults due to a race condition, so I'm leaving that part out for now

@bbpbuildbot

This comment has been minimized.

@codecov-commenter

codecov-commenter commented Mar 21, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.50%. Comparing base (df2343e) to head (8b40594).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1229   +/-   ##
=======================================
  Coverage   86.50%   86.50%           
=======================================
  Files         176      176           
  Lines       13171    13175    +4     
=======================================
+ Hits        11393    11397    +4     
  Misses       1778     1778           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bbpbuildbot

This comment has been minimized.

@JCGoran JCGoran marked this pull request as ready for review March 22, 2024 07:44
@bbpbuildbot

This comment has been minimized.

@ohm314

ohm314 commented Mar 25, 2024

Copy link
Copy Markdown
Contributor

in principle, can be useful, I think. can you actually show the output with --verbose enabled? I still find this doesn't really address the weird issue that Luc pointed out. Because the high-level error message woudl still be confusing.

@1uc

1uc commented Mar 26, 2024

Copy link
Copy Markdown
Collaborator

It certainly doesn't help with #1217 but if it's useful to you, we can merge it.

@bbpbuildbot

Copy link
Copy Markdown
Collaborator

Logfiles from GitLab pipeline #206124 (:white_check_mark:) have been uploaded here!

Status and direct links:

@pramodk

pramodk commented Jun 17, 2024

Copy link
Copy Markdown
Contributor

Looks helpful IMO!

@pramodk pramodk merged commit bf67f44 into master Jun 17, 2024
@pramodk pramodk deleted the jelic/add_logging_parser branch June 17, 2024 12:14
JCGoran added a commit to neuronsimulator/nrn that referenced this pull request Mar 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants