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

[GSoC] Common Python Simulation module #226

Merged
merged 37 commits into from
Sep 25, 2023

Conversation

harshkhandeparkar
Copy link
Collaborator

@harshkhandeparkar harshkhandeparkar commented Aug 9, 2023

Related to #211

Changes

  • Added a common Python simulation module for use across all generators.
  • Used the simulation module to simplify and improve the Temperature Sensor generator simulations.
  • Added documentation (both in Python and in docs/ that is hosted on ReadTheDocs) for the module.
  • Regression Tests

Features of the Simulation Module

  • Easy to sweep through multiple parameters.
  • Generates the output in a well-defined directory structure.
  • Supports Mako templating syntax for inserting parameters in SPICE templates (just like in Verilog templates).
  • Runs concurrent simulations.
  • Reports the simulation status.

Simulations for the DC-DC generator will be based on this.

Screenshots

image

@harshkhandeparkar harshkhandeparkar mentioned this pull request Aug 9, 2023
11 tasks
@harshkhandeparkar harshkhandeparkar marked this pull request as draft August 10, 2023 05:49
@harshkhandeparkar harshkhandeparkar marked this pull request as ready for review August 14, 2023 14:07
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.

I see a lot of changes. Let me review it one by one.

@harshkhandeparkar
Copy link
Collaborator Author

@saicharan0112 any update on this?

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

Copy link
Member

Choose a reason for hiding this comment

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

can this file be renamed in a better way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renaming files might break other flows. @saicharan0112, could you please confirm if it is ok to rename this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes that shouldn't be a problem

Copy link
Member

Choose a reason for hiding this comment

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

We should probably rename this as well.

openfasoc/generators/common/simulation/simulation_run.py Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Are we using Xyce?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is a option to use it (a CLI argument) but I am not sure if it is actually used anywhere or if it is working.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We are installing Xyce as part of the installation script. I remember having problems with the simulations using Xyce but I don't know if there is any progress on that side in general inside OpenFASoC

Copy link
Member

Choose a reason for hiding this comment

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

These are regression tests for the common-api? the naming is kind of weird.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are the expected SPICE files generated from template.sp. There are 8 possible combinations of parameters, and hence 8 different configurations are generated.

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 regression test checks if the correct configurations are generated by comparing the file generated during the testing to the expected file.

Copy link
Collaborator

@saicharan0112 saicharan0112 Sep 9, 2023

Choose a reason for hiding this comment

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

@msaligane I don't think these are actually problems.

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.

@harshkhandeparkar I think the refactoring the simulation part of the generators can be made compatible with the CI. My only concern is the independence of the functions that were previously in separate files such as result.py and result_error.py. Are these refactored functions standalone and can be initialized directly in a different python file or does it expect any declarations that are only known inside the newly developed simulation.py ?

@harshkhandeparkar
Copy link
Collaborator Author

@harshkhandeparkar I think the refactoring the simulation part of the generators can be made compatible with the CI. My only concern is the independence of the functions that were previously in separate files such as result.py and result_error.py. Are these refactored functions standalone and can be initialized directly in a different python file or does it expect any declarations that are only known inside the newly developed simulation.py ?

As far as I remember, the functionality is intact and modular. The functions were moved (and might be renamed) to simulation_error.py, which is now a Python module that can be imported but doesn't run anything. Could you tell which functions from the previous files were required? If they are not available anymore, I'll add them back.

@msaligane
Copy link
Member

@harshkhandeparkar can you pull the changes from the main repo.

@harshkhandeparkar
Copy link
Collaborator Author

@harshkhandeparkar can you pull the changes from the main repo.

Done.

@msaligane msaligane merged commit 76737b5 into idea-fasoc:main Sep 25, 2023
8 checks passed
@saicharan0112
Copy link
Collaborator

@harshkhandeparkar looks like the simulation module changes are not working from the beginning? (another fault of CI is detected thanks to this). not sure how the python tests didn't see this as well. Can you try running the temp-sense-gen again at your side please? Just run make temp-sense-gen and you should be able to see the error while importing the calculate_error... function from simulation_result.py inside the main temp-sense-gen.py file

@harshkhandeparkar
Copy link
Collaborator Author

@harshkhandeparkar looks like the simulation module changes are not working from the beginning? (another fault of CI is detected thanks to this). not sure how the python tests didn't see this as well. Can you try running the temp-sense-gen again at your side please? Just run make temp-sense-gen and you should be able to see the error while importing the calculate_error... function from simulation_result.py inside the main temp-sense-gen.py file

I just tested it and it works on my system locally but it does not work in the openfasoc Docker container. I believe this is because the container uses Python version 3.7.x. Is it possible to bump the version?

Also I think I made a big mistake in #233 which caused the CI to not run at all.

@saicharan0112
Copy link
Collaborator

@harshkhandeparkar looks like the simulation module changes are not working from the beginning? (another fault of CI is detected thanks to this). not sure how the python tests didn't see this as well. Can you try running the temp-sense-gen again at your side please? Just run make temp-sense-gen and you should be able to see the error while importing the calculate_error... function from simulation_result.py inside the main temp-sense-gen.py file

I just tested it and it works on my system locally but it does not work in the openfasoc Docker container. I believe this is because the container uses Python version 3.7.x. Is it possible to bump the version?

Also I think I made a big mistake in #233 which caused the CI to not run at all.

Yes but I handled it in #240 . Regarding the python version, I am not sure. The entire environment is kind of pinned to 3.7. I think there is a way to use 3.8 (something that I am looking into #237) but 3.10 is something I am not sure about.

@saicharan0112
Copy link
Collaborator

I think I am able to find a way to install all tools in python 3.10. I still need to test the generators with latest openroad (not sure how big that will blow) but I will update the docker image to give you the python version that you need for your code.

@harshkhandeparkar
Copy link
Collaborator Author

I think I am able to find a way to install all tools in python 3.10. I still need to test the generators with latest openroad (not sure how big that will blow) but I will update the docker image to give you the python version that you need for your code.

I can try to make it compatible with 3.7.

@saicharan0112
Copy link
Collaborator

I think I am able to find a way to install all tools in python 3.10. I still need to test the generators with latest openroad (not sure how big that will blow) but I will update the docker image to give you the python version that you need for your code.

I can try to make it compatible with 3.7.

no 3.7 is quite old and miniconda not supporting 3.7 is a sign for developers not use it anymore. Also since now all dependencies are working in 3.10 env, there should be no problem in handling such stuff.

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.

3 participants