Skip to content

PR for #139: Improving logging to avoid concurrency issues. #167

Merged
liaochris merged 82 commits into
mainfrom
139-sconscriptlog-does-not-capture-logs-from-multiple-scripts
Jun 19, 2026
Merged

PR for #139: Improving logging to avoid concurrency issues. #167
liaochris merged 82 commits into
mainfrom
139-sconscriptlog-does-not-capture-logs-from-multiple-scripts

Conversation

@liaochris

@liaochris liaochris commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

In this issue, we implemented a new logging procedure that replaces subfolder-widesconscript.log files with script-specific files in a log folder that mirrors source and is not version-controlled.

zhizhongpu and others added 30 commits February 28, 2026 09:48
Comment thread output/run.csv Outdated

@zhizhongpu zhizhongpu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're just waiting for @arewolinski to take a look.

@zhizhongpu

Copy link
Copy Markdown
Collaborator

@toogiii @arewolinski I hope you can both make sure that scons runs well both clean (after scons --clean) and unclean, before you approve the pull.

This is especially important as my machine sometimes runs into a known issue of matlab hanging

@liaochris do you think we want to execute a run on SLURM too, just in case? If so, we might as well have @arewolinski and @toogiii do it since running the Template is typically the onboarding task for SLURM.

@toogiii

toogiii commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

happy to try the slurm run tomorrow. the scons and scons --clean calls still work for me locally

@liaochris

liaochris commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator Author

@liaochris do you think we want to execute a run on SLURM too, just in case?

@zhizhongpu works when I compile Template on slurm.

FYI @arewolinski @toogiii you'll probably encounter some missing LaTeX .sty file errors when you attempt to compile on the RC. The RC has instructions on how you can install LaTeX packages to overcome those errors, but if you encounter difficulty finding that documentation please let me know.

@liaochris

liaochris commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

@toogiii @arewolinski I think your reviews are the last thing blocking this pull. Here's what I think we need to do in order to merge the pull.

  1. Both of you still need to compile the Template on the RC. This should be a similar procedure to compiling the Template on a local machine.

  2. @arewolinski I think you still need to leave review comments - please see the pull review guidelines below

* _Reviewers_ **review** the deliverable.
* Check for violations of our standards for code clarity and data integrity (production deliverables).
* Check for clear errors or reasons reproducibility will fail (all deliverables.)
* _Reviewers_ **approve** the pull when all pull threads are resolved.

Compiling the template on the RC should cover point (2) above.

Let me know if you have any questions about this, and when you have done the tasks above, you can approve the pull.

@toogiii toogiii self-requested a review June 12, 2026 20:38
Comment thread .sconsign.dblite

@toogiii toogiii left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only one thing @liaochris: should we add .sconsign.dblite to .gitignore? is there a reason we would want to git add this ever?

@toogiii toogiii left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved other review.

@liaochris

Copy link
Copy Markdown
Collaborator Author

/run-actions-all --post

@github-actions

Copy link
Copy Markdown

Triggered by @liaochris on this comment

Check Results (run details)

Check Result Time
SCons DAG 0.037s
Newlines 0.045s
EPS data 0.024s
Log failures SKIP

@liaochris liaochris merged commit 1948562 into main Jun 19, 2026
18 checks passed
@github-actions

Copy link
Copy Markdown

@liaochris Issue summary

Thanks for closing this pull.

Before leaving the pull, please be sure you have completed all the required steps in the workflow.

This includes filling in the issue summary linked at the top of this comment.

@liaochris liaochris deleted the 139-sconscriptlog-does-not-capture-logs-from-multiple-scripts branch June 19, 2026 15:29
@zhizhongpu

Copy link
Copy Markdown
Collaborator

@liaochris is the SKIP here #167 (comment) expected?

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.

sconscript.log does not capture logs from multiple scripts

6 participants