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.

Sign up for GitHub

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

Copy simulation_inputs | add docs #261

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75fa5f9
Copy simulation_inputs | add docs
joonho3020 Jan 23, 2023
338966e
fix cp -> shutil.copy | docs
joonho3020 Jan 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions boards/default/installers/firesim/firesim.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@ def install(targetCfg, opts):
if 'simulation_outputs' in jCfg['firesim']:
wls[slot]["simulation_outputs"] = [f.as_posix() for f in jCfg['firesim']['simulation_outputs']]
if 'simulation_inputs' in jCfg['firesim']:
wls[slot]["simulation_inputs"] = [f.as_posix() for f in jCfg['firesim']['simulation_inputs']]
wls[slot]["simulation_inputs"] = [os.path.basename(f.as_posix()) for f in jCfg['firesim']['simulation_inputs']]
for f in jCfg['firesim']['simulation_inputs']:
os.system(f"cp {f} {os.path.join(fsTargetDir, os.path.basename(f))}")
joonho3020 marked this conversation as resolved.
Show resolved Hide resolved

fsCfg['workloads'] = wls
else:
Expand All @@ -88,7 +90,9 @@ def install(targetCfg, opts):
if 'simulation_outputs' in targetCfg['firesim']:
fsCfg["common_simulation_outputs"] = [f.as_posix() for f in targetCfg['firesim']['simulation_outputs']]
if 'simulation_inputs' in targetCfg['firesim']:
fsCfg["common_simulation_inputs"] = [f.as_posix() for f in targetCfg['firesim']['simulation_inputs']]
fsCfg["common_simulation_inputs"] = [os.path.basename(f.as_posix()) for f in targetCfg['firesim']['simulation_inputs']]
for f in targetCfg['firesim']['simulation_inputs']:
os.system(f"cp {f} {os.path.join(fsTargetDir, os.path.basename(f))}")

with open(str(fsTargetDir / "README"), 'w') as readme:
readme.write(readmeTxt)
Expand Down
19 changes: 19 additions & 0 deletions docs/source/workloadConfig.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,25 @@ should be absolute with respect to the workload rootfs. Files will be placed
together in the output directory. You cannot specify the directory structure of
the output.

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 •
edited
Loading

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 :)


simulation_inputs
^^^^^^^^^^^^^^^^^^^^^
A list of simulation input files to provide to firesim before running simulations.
Each path should be relative to the firemarshal top directory.

.. Note:: `simulation_outputs` and `simulation_inputs` are specific to firesim
workloads and hence should be wrapped with the `firesim` key.
```
firesim: {
simulation_inputs: ["./images/br-base/br-base-bin-dwarf"],
simulation_outputs: ["memory_stats0.csv", "metasim_stderr.out"]
}
```

.. _workload-rootfs-size:

rootfs-size
Expand Down