[Autotuner] - Tunable variables check #2452
Conversation
docs/user/FlowVariables.md
Outdated
| @@ -100,6 +100,7 @@ configuration file. | |||
| | <a name="GPL_TIMING_DRIVEN"></a>GPL_TIMING_DRIVEN| Specifies whether the placer should use timing driven placement.| | | |||
| | <a name="GUI_TIMING"></a>GUI_TIMING| Load timing information when opening GUI. For large designs, this can be quite time consuming. Useful to disable when investigating non-timing aspects like floorplan, placement, routing, etc.| | | |||
| | <a name="HOLD_SLACK_MARGIN"></a>HOLD_SLACK_MARGIN| Specifies a time margin for the slack when fixing hold violations. This option allows you to overfix.| | | |||
| | <a name="IO_CONSTRAINTS"></a>IO_CONSTRAINTS| File path to the IO constraints .tcl file.| | | |||
There was a problem hiding this comment.
I propose not updating FlowVariables.md in pull requests that modify generate-variables-docs.py because it is likely to cause conflicts with other PRs in flight.
| # information about the variables from variables.yaml. | ||
| """ | ||
| This script injects an autogenerated section in FlowVariables.md with | ||
| information about the variables from variables.yaml. |
There was a problem hiding this comment.
break out update to generate-variables-docs.py to separate PR, it has nothing to do with AutoTuner.
| ] | ||
| def load_yaml(yaml_path: str) -> dict: | ||
| if not os.path.exists(yaml_path): | ||
| raise FileNotFoundError(f"File {yaml_path} not found") |
There was a problem hiding this comment.
Interesting: why are you checking here instead of letting open() propagate this exception? What is the difference?
Seems like this code should be removed, or at least it is very surprising and I have never seen it.
If there is a reason to keep it, it deserves a comment.
There was a problem hiding this comment.
I usually try to write this to catch the exact exception triggering the open() error. Makes the code slightly verbose but easier to debug. 99% of the time open(... , "r") do fail with FileNotFoundError, but there might be other exceptions out there.
Also yaml.safe_load() might fail too (e.g. invalid yaml)
There was a problem hiding this comment.
I think it is better to leave file not found to open(), it is what all other python code i have seen does: minimize surprises and questions in the code.
there is no evidence of specific unique circumstances here.
luarss
force-pushed
the
topic/at-flow-var
branch
from
8848846
to
37c7d9c
Compare
luarss
changed the title
luarss
force-pushed
the
topic/at-flow-var
branch
from
c1dc0b4
to
08a132b
Compare
| variables.add(variable.strip().upper()) | ||
| # Read from variables.yaml and get variables with tunable = 1 | ||
| os.chdir(vars_path) | ||
| with open("variables.yaml") as file: |
There was a problem hiding this comment.
don't use os.chdir(), use full paths instead.
Signed-off-by: Jack Luar <[email protected]>
Signed-off-by: Jack Luar <[email protected]>
Signed-off-by: Jack Luar <[email protected]>
Signed-off-by: Jack Luar <[email protected]>
Signed-off-by: Jack Luar <[email protected]>
Signed-off-by: Jack Luar <[email protected]>
luarss
force-pushed
the
topic/at-flow-var
branch
from
9953874
to
d93180d
Compare
| # if key not in flow_variables: | ||
| # print(f"[ERROR TUN-0017] Variable {key} is not tunable.") | ||
| # sys.exit(1) | ||
| if flow_variables.get(key, 0) == 0: |
There was a problem hiding this comment.
use a set and "key in flow_variables", easier to read
Signed-off-by: Jack Luar <[email protected]>
PR Goals
Fixes Autotuner: Make flow variable detection more robust #2401
In future PRS