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

Copy simulation_inputs | add docs #261

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

joonho3020
Copy link
Contributor

Address comments on #260.

  • Copy simulation_inputs into the workload slot directory.
  • Add documentation.

simulation_outputs
^^^^^^^^^^^^^^^^^^^^^
A list of simulation output files to copy after running firesim. Each path
should be absolute with respect to the `sim_slot_*` directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Nit: FireSim instead of firesim.
  • I think this documentation exposed something I didn't see before. Right now sim. inputs are from the marshal root directory. However, I think there might be cases in the future where things are taken from the workdir (relative to the workdir). I think a good extension of this would be to enable inputs from multiple sources: 1. relative to the marshal root directory 2. relative to the workdir 3. absolute path to anywhere. To DRY this out there can be a shared func that takes list of locations to look for, list of files to find (both absolute and relative) and returns the absolute paths of all files. Then you can take the output of this function to pass into some copy loop (and/or error).

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Nit: We should probably have a new section/header of the docs with simulator-specific flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think the workdir thing will be okay in cases when the user specifies a filename & there are multiple files within the search scope with the same name?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if there are multiple files then there should be an error instead of silently resolving to a specific file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll get back to this a bit later :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My impression of the path thing is that the user should know & specify the path marshal should search for each input file. So instead of searching for random locations, shouldn't we group each input file by the base path we want to search for?
Something like this?

"simulation_inputs" : {
  "marshal" : [ "input1", "input2"],
  "workload" : [ "input3", "input4"]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this adds more verboseness to the file than needed.

Maybe we can simplify this to be... search in:

  • image dir that is generated for the workload
  • work dir that is specified
  • absolute paths

Why do you need access to the top-level marshal dir?

Copy link
Contributor Author

@joonho3020 joonho3020 Feb 3, 2023

Choose a reason for hiding this comment

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

I don't need access to the top-level marshal dir. Rather, I thought it would be cleaner to explicitly specify where to copy files from & for consistency in the above example. I guess, I kind of don't like the idea of searching for files for a set of relative paths. I think absolute paths would be great though. I guess I can search through those structures though :)

boards/default/installers/firesim/firesim.py Outdated Show resolved Hide resolved
@joonho3020 joonho3020 self-assigned this Feb 1, 2023
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.

2 participants