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

Added some missing flow variables and descriptions #2555

Merged

Conversation

jeffng-or
Copy link
Contributor

No description provided.

@jeffng-or jeffng-or requested a review from maliberty November 8, 2024 22:14
Copy link
Collaborator

@oharboe oharboe left a comment

Choose a reason for hiding this comment

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

Just to check: This documentation is autogenerated by scripts/generatedocs.py

Did you update variables.py and rerub scripts/generatedocs.py?

@jeffng-or
Copy link
Contributor Author

jeffng-or commented Nov 9, 2024

Just to check: This documentation is autogenerated by scripts/generatedocs.py

Did you update variables.py and rerub scripts/generatedocs.py?

Thanks for the pointer. I'll update variables.yaml and the generate-variables-docs.py script.

@jeffng-or jeffng-or closed this Nov 9, 2024
@jeffng-or jeffng-or reopened this Nov 9, 2024
@jeffng-or
Copy link
Contributor Author

Updated variables.yaml and re-generated FlowVariables.md. It looks like FlowVariables.md wasn't updated in a while, since there are a bunch of new additions in the generated file.

@oharboe
Copy link
Collaborator

oharboe commented Nov 9, 2024

@jeffng-or @maliberty Autogenerating .md files under git source control is not great, but github does not have any dynamic rendering capabilities in .md files.

So, pick a poison: keep generated docs in git, or you dont get .md docs displayed when browsing github and the user needs to consult some other source.

@oharboe
Copy link
Collaborator

oharboe commented Nov 9, 2024

Unrelated to this PR that is just regenerating:

RTLMP_FLOW should be deleted from variables.yaml

USE_FILL default in variables.yaml needs a look

@oharboe
Copy link
Collaborator

oharboe commented Nov 9, 2024

@maliberty @jeffng-or I am of two minds of too much complication in CI for obscure cases, but it would be possible to check that FlowVariables.md, if changed, is not different after scripts/generatedocs.py has been run: this would cause failure when a PR manually modiies the autogenerated section.

@maliberty
Copy link
Member

I imagine something like "Check That ODB Files Are Generated" in OR could be used for the docs. @vvbandeira @luarss any thoughts?

@maliberty
Copy link
Member

I don't see any change to variables.yaml in this PR

@luarss
Copy link
Contributor

luarss commented Nov 9, 2024

@maliberty There is some initial CI (WIP) for variables.yaml here: #2457

@oharboe
Copy link
Collaborator

oharboe commented Nov 9, 2024

@maliberty There is some initial CI (WIP) for variables.yaml here: #2457

Thanks for the reference!

@jeffng-or
Copy link
Contributor Author

I don't see any change to variables.yaml in this PR

Sorry, committed to the wrong branch. Updated the PR.

@maliberty
Copy link
Member

Sorry, committed to the wrong branch. Updated the PR.

I don't see any update - did you push?

@maliberty maliberty merged commit 5e33163 into The-OpenROAD-Project:master Nov 12, 2024
4 of 6 checks passed
@jeffng-or jeffng-or deleted the flow-variable-doc-updates branch November 12, 2024 17:02
@oharboe
Copy link
Collaborator

oharboe commented Nov 13, 2024

@jeffng-or @maliberty Defaults can originate in ORFS or OpenROAD.

If they originate in ORFS, the single - source of truth is variables.yaml

However, if the default values originate in OpenROAD, there is no way to automatically update FlowVariables.md, nor ensure that variables.yaml is up to date.

The choices I can think of are:

  1. duplicate information into variables.yaml. Fine as long as default never changes in OpenROAD. This is done today, but not consistently.
  2. do not document default in variables.yaml: the user is forced to look up default in OpenROAD. The advantage is that the value is known to be correct(single source of truth).
  3. seperate the concern of defaults in OpenROAD and ORFS. Modify ORFS to always specify OpenROAD option with default value from variables.yaml

As for recommendation, I am inclined to leaving as-is and switching to 3 for those cases where it turns out that it matters to anyone.

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.

4 participants