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

error handling and submit_command refactoring (with tests for synthesis) #527

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

ssteffl
Copy link
Contributor

@ssteffl ssteffl commented Oct 1, 2019

other noteworthy changes include:

  • added lazytrycrossref and lazytrysubst meta types to hammer-config (with tests)
  • hammer-ir keys for filtering to nldm and subset of stdcell libs for debugging
  • added more sdc constraint hammer-ir keys, and to sdc generation
  • added time_unit and capacitance_unit to sdc, and removed current workaround
  • added is_physical key for synthesis, to toggle lef/def usage

other minor changes include:

  • more readable syn.tcl formatting
  • added CapacitanceValue unit for default_load
  • added more timing/linting reports to genus flow

@jwright6323
Copy link
Contributor

After an extremely quick glance this looks good, but I'll dive a little deeper tomorrow.

Copy link
Member

@edwardcwang edwardcwang left a comment

Choose a reason for hiding this comment

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

Generally looks good overall, thanks for the PR!

One thing I might note is that there's a lot going on here - if possible, perhaps it could be possible to refactor this into multiple smaller PRs that are easier to look at in isolation?

src/hammer-tech/filters.py Outdated Show resolved Hide resolved
src/hammer-tech/filters.py Outdated Show resolved Hide resolved
for vt in vts:
for provided in lib.provides:
has_vt = has_vt or (provided.vt == vt)
if not(has_vt):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if not(has_vt):
if not has_vt:

src/hammer-vlsi/defaults.yml Show resolved Hide resolved
Comment on lines +99 to +102
# how many output lines to buffer in memory. "" for unlimited
max_output_lines: null
# how many error lines to buffer in memory. "" for unlimited
max_error_lines: null
Copy link
Member

Choose a reason for hiding this comment

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

This is meant to be a suggestion for all 4 lines but I think GitHub doesn't know how to do it for all 4 lines...

Suggested change
# how many output lines to buffer in memory. "" for unlimited
max_output_lines: null
# how many error lines to buffer in memory. "" for unlimited
max_error_lines: null
# How many output lines to buffer in memory.
# If unset, buffer unlimited lines.
# type: Optional[int]
max_output_lines: null
# How many error lines to buffer in memory.
# If unset, buffer unlimited lines.
# type: Optional[int]
max_error_lines: null

""", is_yaml=True)
db.update_core([base, meta])
self.assertEqual(db.get_setting("lazy.numbers"), ["1", "2", "3"])
self.assertEqual(db.get_setting("lazy.numbers2"), [1,2,3])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.assertEqual(db.get_setting("lazy.numbers2"), [1,2,3])
self.assertEqual(db.get_setting("lazy.numbers2"), [1, 2, 3])

src/hammer_config/config_src.py Outdated Show resolved Hide resolved
"""Substitute ${...}"""

# this check allows a reference like "${foo}" to just be replaced
# instead of using regexes. therefore substituting int/bool/null works
Copy link
Member

Choose a reason for hiding this comment

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

How does this differ from crossref?

If "abc ${foo}" is trysubsted, how does it work if foo is an int or bool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it breaks. i found no good alternative there.

but the common case is when a key refers to "${foo}", I want this to work regardless of what the type of the setting it.

the only difference between trysubst and subst, is in the subst_target() function:
the trysubst will simply return no targets if it detects a non str/List[str] setting value, which is what I want. I left the default behaviour of subst alone.

I would prefer to just have subst, and change the behavior to allow non-string setting values instead of raising an exception.

src/hammer_config/config_src.py Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
submit_command.sh
Copy link
Member

Choose a reason for hiding this comment

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

Not sure as to why this is here...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the HammerSubmitCommand will always drop a self-contained command script and a log file in the run-directory. In haste, i just hard-coded the command script to submit_command.sh. when i run tests, that file got dropped in the hammer work-tree.

if we ever want to run multiple hammerSubmitCOmmands in the same working-directory, this will have to be fixed (i'm assuming it will)

ssteffl and others added 5 commits October 1, 2019 19:38
Co-Authored-By: edwardcwang <edwardcwang@users.noreply.github.com>
Co-Authored-By: edwardcwang <edwardcwang@users.noreply.github.com>
Co-Authored-By: edwardcwang <edwardcwang@users.noreply.github.com>
Co-Authored-By: edwardcwang <edwardcwang@users.noreply.github.com>
Co-Authored-By: edwardcwang <edwardcwang@users.noreply.github.com>
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