Skip to content
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

Store simulation result file, return sim state from run #270

Merged
merged 30 commits into from
Dec 8, 2023
Merged

Store simulation result file, return sim state from run #270

merged 30 commits into from
Dec 8, 2023

Conversation

chetanyagoyal
Copy link
Collaborator

@chetanyagoyal chetanyagoyal commented Nov 9, 2023

Stores the simulation result file (prePEX_sim_result) to the new directory in.github/scripts and returns the state of simulations at the end of the simulation run to a file in the work folder

@chetanyagoyal chetanyagoyal mentioned this pull request Nov 9, 2023
3 tasks
Copy link
Collaborator

@saicharan0112 saicharan0112 left a comment

Choose a reason for hiding this comment

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

This is nothing functional but I would strongly suggest to created a README file under expected_sim_outputs with info about these generatd simulation results (like the tool versions that you used and the temp-sense generator config). Also, the file name is very generic. How do we know which generator does it belong to?

with open(result_filename) as f2, open(template_filename) as f1:
content1 = f2.readlines()
content2 = f1.readlines()
if content1 != content2:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will the content be exactly the same each time? The numbers are very precise and may probably vary slightly in each run?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah they might vary, that's why it isnt a hard requirement for the script to succeed

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the hard requirement?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the number of failed simulations should be 0 (returned in the dictionary at the end of simulation flow)

.github/scripts/parse_rpt.py Outdated Show resolved Hide resolved
.github/scripts/expected_sim_outputs/prePEX_sim_result Outdated Show resolved Hide resolved
@@ -39,7 +39,7 @@ jobs:
cp ./.github/scripts/parse_rpt.py ./openfasoc/generators/temp-sense-gen/. &&\
pip3 install -r requirements.txt &&\
cd ./openfasoc/generators/temp-sense-gen &&\
make sky130hd_temp &&\
make sky130hd_temp_full &&\
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest keeping flow till drc/lvs checks and the full flow till simulation checks separately. It would be easy to understand which part is failing instead of digging into the log files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean that I should have one job with make .. and another job with make ..full?

Copy link
Collaborator

@saicharan0112 saicharan0112 left a comment

Choose a reason for hiding this comment

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

LGTM

@chetanyagoyal
Copy link
Collaborator Author

@msaligane I think this can be merged now

@msaligane msaligane merged commit b70f754 into idea-fasoc:main Dec 8, 2023
4 of 8 checks passed
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.

4 participants