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

fixed sdc file copy and parmys abs path to temp folder issue #2427

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

WhiteNinjaZ
Copy link
Contributor

This PR resolves issue #2420 and provides a work around to #2302.

Description

The run_vtr_flow.py script was modified to properly copy a specified sdc file into a temp directory regardless of if a relative or absolute path is given to the sdc file. Also, this PR provides a work around for the new requirement by parmys that the path specified for the temp directory be an absolute path. The updated script provides the flow with an absolute path whether an absolute or relative path is specified. Additionally, several command line options are specified in the documentation as having a double -- rather than a single - (namely fixed_pins and sdc_file). The script was modified to provide consistency between how these options are used in both the vpr executable and the script.

How Has This Been Tested?

Several designs were run through the flow using -temp and --sdc_file files with both relative and absolute paths.

Types of changes

  • Bug fix (change which fixes an issue)
  • New feature (change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@vaughnbetz
Copy link
Contributor

Not sure why CI failed -- @WhiteNinjaZ could you relaunch this?

Copy link
Contributor

@vaughnbetz vaughnbetz left a comment

Choose a reason for hiding this comment

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

@WhiteNinjaZ : I have a question about the absolute path requirement.

vtr_flow/scripts/run_vtr_flow.py Show resolved Hide resolved
@vaughnbetz
Copy link
Contributor

@WhiteNinjaZ : It would be good to move this PR along ... see questions and CI failures above.

@github-actions github-actions bot added the lang-python Python code label Jun 4, 2024
@WhiteNinjaZ
Copy link
Contributor Author

Will do, it looks like the logs expired for most of the CI tests so re-running now.

@vaughnbetz
Copy link
Contributor

Some jobs timed out ... maybe due to busy CI lately. Re-launching the failed jobs.

@vaughnbetz
Copy link
Contributor

Not sure why we have some CI failures; relaunching.

@vaughnbetz
Copy link
Contributor

It seems like some tests timed out again, and I can't find the usual button to relaunch the tests. @WhiteNinjaZ @AlexandreSinger : any ideas? This change looks pretty safe (scripting update) given that most CI tests passed and others seem to have timed out. If you can figure out how to relaunch that would be safest, but if @WhiteNinjaZ is pretty sure this works with all tests I could also just merge.

@vaughnbetz
Copy link
Contributor

Ah, didn't think of that. Thanks @AlexandreSinger

@AlexandreSinger
Copy link
Contributor

@vaughnbetz I also saw this PR and was curious why I could not rerun the CI. I believe the reason is because the CI clears runs after a certain period of time (I think a couple of weeks). If a CI run fails for a PR, the CI cannot be rerun the usual way if its been too long (for this PR, the last run was in June).

Normally when this happens I rebase the branch onto Master; however for some reason that button does not work for me on this PR. It may be because the branch is under a different fork (that I do not have the right to modify). So the button to rebase / merge with master was not here for me.

To get around this issue I just closed and reopened the PR. That relaunched the CI. Kinda goofy, but it works. If it fails CI again, I recommend @WhiteNinjaZ to rebase his branch. I am a bit worried about this change in that maybe some CI tests rely on the CLI options to be a certain way and its causing timeouts. The CI will tell.

@vaughnbetz
Copy link
Contributor

Thanks @AlexandreSinger . Yes, I was a little worried in case there was something unexpected happening in the tests due to the scripting changes.

@AlexandreSinger
Copy link
Contributor

@vaughnbetz Yeah there is something in this PR that is causing the strong tests to time out. My guess is the use of os.getcwd(). These scripts are often run all over the place, so I am not sure if we can guarantee that the current working directory is where we think it is. Looking through the strong tests there are a lot of failures going on. Perhaps this should be rebased onto master and tested locally.

@vaughnbetz
Copy link
Contributor

Thanks @AlexandreSinger . @WhiteNinjaZ : can you try this out locally for the strong tests as Alex suggests? Something is going wrong with the strong tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang-python Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants