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

Use conda + Update initial setup docs #1163

Merged
merged 25 commits into from
Sep 16, 2022
Merged

Use conda + Update initial setup docs #1163

merged 25 commits into from
Sep 16, 2022

Conversation

abejgonzalez
Copy link
Contributor

@abejgonzalez abejgonzalez commented Apr 28, 2022

This PR removes the *-req.sh scripts in favor of using Conda (same system that FireSim uses to manage system packages). I've also changed the documentation to be a bit clearer on how to run things in a AWS EC2 context and in the normal scenario. Notable changes are below:

  • Use conda for package management. Set of working packages are listed in conda-requirements*.yaml files. Additionally, lock files *.conda-lock.yml files are provided to ensure a consistent set of packages.
  • New setup.sh file that automates the setup steps including setting up a conda environment, submodule cloning, and building extra toolchain collateral (i.e. Spike, PK, etc)
  • Bump to Python 3.9 for FireMarshal (tested buildroot/fedora build with it). Also, use QEMU built by conda in the ucb-bar channel.
  • Now using toolchain packages uploaded to ucb-bar conda channel (riscv-tools and esp-tools are both provided)
  • Updated initial setup documentation. Point to FireSim/Chipyard docs in specific places. See https://chipyard.readthedocs.io/en/conda/Chipyard-Basics/Initial-Repo-Setup.html#initial-repository-setup
  • Updated CI to use self-hosted runners for test runs. Needed since GH-A cache action now doesn't support caching between Docker containers and self-hosted runners.

Related PRs / Issues:

Type of change:

  • Bug fix
  • New feature
  • Other enhancement

Impact:

  • RTL change
  • Software change (RISC-V software)
  • Build system change
  • Other

Contributor Checklist:

  • Did you set main as the base branch?
  • Is this PR's title suitable for inclusion in the changelog and have you added a changelog:<topic> label?
  • Did you state the type-of-change/impact?
  • Did you delete any extraneous prints/debugging code?
  • Did you mark the PR with a changelog: label?
  • (If applicable) Did you add documentation for the feature?
  • (If applicable) Did you add a test demonstrating the PR?
  • (If applicable) Did you mark the PR as Please Backport?

Copy link
Contributor

@jerryz123 jerryz123 left a comment

Choose a reason for hiding this comment

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

This seems mostly fine.
I would reduce the amount of stuff the machine-launch-script.sh has to do. If we assume Conda is installed already, than the script does not need SUDO, and it can be renamed to conda-env-setup.sh or something (which is much more descriptive than "machine launch").

We should generally avoid giant multi-purpose scripts with a bunch of flags that leave effects on the user's environment. This is really only acceptable for the F1 case, where the instances are "disposable".

@abejgonzalez
Copy link
Contributor Author

I've sketched out where the conda activate calls should go. Once that is settled I'll update the docs to reflect that.

Copy link
Contributor

@jerryz123 jerryz123 left a comment

Choose a reason for hiding this comment

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

Looks close, but I think with some changes we can support the no-conda use case too.

conda activate firesim
else
conda activate chipyard
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

When would there be a firesim env? Why not just tell the user to always conda activate chiypard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FireSim by default calls it's env firesim. So if we want to support Chipyard in the FireSim setup (on AWS F1) then we should switch between them.

Copy link
Contributor

Choose a reason for hiding this comment

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

The docs say

Warning:: When running on an Amazon Web Services EC2 FPGA-development instance (for FireSim), FireSim includes a similar machine setup script that will install all of the aforementioned dependencies (and some additional ones). Skip this section.

This instructs users running Chipyard in FireSim to skip running this script, since I believe the fsim docs should contain instructions for activating the firesim env. In that case, this script doesn't need to exist, and the docs can just say conda activate chipyard

For my own understanding, I believe the flow is:

  1. If FireSim, follow firesim docs, then skip to 5
  2. install conda
  3. create chipyard conda env
  4. activate conda env
  5. init submodules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was re-worded in the past. Better now?

@abejgonzalez
Copy link
Contributor Author

Cancelling CI since there needs to be more work done to make that work.

.github/CI_README.md Outdated Show resolved Hide resolved
@abejgonzalez
Copy link
Contributor Author

This PR is good to merge. The errors for SPI flash/read are because we need to bump Verilator to 4.226 and the CVA6 error is because there is a segfault when CVA6 is used with DRAMSim2 in the new verilator (this is solved if you use magic memory)

@jerryz123
Copy link
Contributor

CVA6 with VCS + DRAMSIM is still fine?

@abejgonzalez
Copy link
Contributor Author

abejgonzalez commented Sep 13, 2022

CVA6 with VCS + DRAMSIM is still fine?

I haven't had the chance to verify this on this PR but pre-Conda I do remember this working. I can confirm that VCS + CVA6 + DRAMSim2 works

Copy link
Member

@sagark sagark left a comment

Choose a reason for hiding this comment

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

LGTM modulo a few fixes in comments.

@abejgonzalez
Copy link
Contributor Author

CVA6 with VCS + DRAMSIM is still fine?

I haven't had the chance to verify this on this PR but pre-Conda I do remember this working. I can confirm that VCS + CVA6 + DRAMSim2 works

I've gone ahead and fixed this by removing a getenv call in DRAMSim2 (which I don't think is used often)(the getenv is causing a segfault presumably due to multi-thread race condition).

abejgonzalez and others added 3 commits September 15, 2022 11:42
Co-authored-by: Sagar Karandikar <sagark@eecs.berkeley.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants