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

Jump to bottom

docs: autogenerate flow variables table #2416

Merged
merged 4 commits into from
Oct 9, 2024
Merged

docs: autogenerate flow variables table #2416

merged 4 commits into from
Oct 9, 2024

Conversation

oharboe
Copy link
Collaborator

@oharboe oharboe commented Oct 3, 2024

No description provided.

@oharboe oharboe marked this pull request as draft October 3, 2024 19:34
@oharboe oharboe changed the title docs: autogenerate flow variables table [NOT READY FOR REVIEW] docs: autogenerate flow variables table Oct 3, 2024
@oharboe
Copy link
Collaborator Author

oharboe commented Oct 3, 2024

@maliberty Something like this will be a first pass, but I need to flesh out the metadata a bit to get back to more or less where FlowVariables.md used to be at.

@oharboe oharboe force-pushed the variables-docs branch 4 times, most recently from 61da84d to ad51b75 Compare October 4, 2024 06:34
@oharboe oharboe marked this pull request as ready for review October 4, 2024 08:06
@oharboe
docs: autogenerate flow variables table
81057c6 
- erase variables that should not be used for a stage
- generate doc
- initial pass at assigning variables to stages, no stages
  listed puts the variables into the "all" category and
  the variables are never erased.

Signed-off-by: Øyvind Harboe <[email protected]>
@oharboe oharboe changed the title [NOT READY FOR REVIEW] docs: autogenerate flow variables table docs: autogenerate flow variables table Oct 4, 2024
@oharboe
Copy link
Collaborator Author

oharboe commented Oct 4, 2024 •
edited
Loading

@maliberty Thoughts?

ci pr-merge seems unrelated.

@oharboe oharboe requested a review from maliberty October 5, 2024 07:04
@oharboe
Copy link
Collaborator Author

oharboe commented Oct 5, 2024

@maliberty Flaky autotuner test in pr-merge? It went away last time with a retriggered build?

maliberty
maliberty requested changes Oct 7, 2024
View reviewed changes
flow/scripts/variables.py Outdated Show resolved Hide resolved
flow/scripts/stage_variables.py Outdated Show resolved Hide resolved
@maliberty
Copy link
Member

image

is not very readable and also misleading (eg it suggests synthesis depends on the KLAYOUT_TECH_FILE which it clearly doesn't).

@oharboe
Copy link
Collaborator Author

oharboe commented Oct 7, 2024

image

is not very readable and also misleading (eg it suggests synthesis depends on the KLAYOUT_TECH_FILE which it clearly doesn't).

Fixed misleading bit: only list variables if they have the stage in "stages:".

As to more readable...

Suggestions?

This is a summary of variables for that stage.

@maliberty
Copy link
Member

Fixed misleading bit: only list variables if they have the stage in "stages:".

Did you push this?

@oharboe
Copy link
Collaborator Author

oharboe commented Oct 7, 2024

Fixed misleading bit: only list variables if they have the stage in "stages:".

Did you push this?

No, I'm writing up comments as I fix, then I will push. There's no great workflow for where to keep such comments in "draft" until I have pushed...

@oharboe oharboe requested a review from maliberty October 7, 2024 19:13
@oharboe
Copy link
Collaborator Author

oharboe commented Oct 7, 2024

@maliberty As to when re-review: I retrigger a review after updating the PR, so I guess that is the best that I know of w.r.t. the review being ready for another review. Generally ignore noise from github until then...

@maliberty
Copy link
Member

I think generating a bullet list of variables for each stage would be more readable. If they could link back to their definition that would be helpful.

I would be nice to have a list of variables that are "all" and those that are uncategorized. It will be easier to push those forward if there is a list.

@maliberty
Copy link
Member

Once this is all settled I'll run a full CI as the erased envars need testing in all flows.

@oharboe
Copy link
Collaborator Author

oharboe commented Oct 7, 2024

I think generating a bullet list of variables for each stage would be more readable.

That would give you doom scrolling, no?

@oharboe
variables.yaml: review feedback
a89296c 
Signed-off-by: Øyvind Harboe <[email protected]>
@oharboe
Copy link
Collaborator Author

oharboe commented Oct 7, 2024

I think generating a bullet list of variables for each stage would be more readable. If they could link back to their definition that would be helpful.

I would be nice to have a list of variables that are "all" and those that are uncategorized. It will be easier to push those forward if there is a list.

Fixed

@maliberty
Copy link
Member

Secure CI started.

@maliberty
Copy link
Member

The CI is fine but the lint error needs fixing.

@oharboe
lint: run black
8abc4cf 
Signed-off-by: Øyvind Harboe <[email protected]>
@oharboe
Copy link
Collaborator Author

oharboe commented Oct 8, 2024

The CI is fine but the lint error needs fixing.

Fixed.

@oharboe oharboe requested a review from maliberty October 8, 2024 14:00
@oharboe
Copy link
Collaborator Author

oharboe commented Oct 9, 2024

I prefer to have this merged before I update docs in other PRs....

@maliberty maliberty merged commit faa7570 into The-OpenROAD-Project:master Oct 9, 2024
5 of 6 checks passed
@oharboe oharboe deleted the variables-docs branch October 10, 2024 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maliberty maliberty Awaiting requested review from maliberty

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@oharboe @maliberty