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

Flow breaks when constant name changes in design_top.json #3

Closed
steveri opened this issue Apr 8, 2017 · 19 comments
Closed

Flow breaks when constant name changes in design_top.json #3

steveri opened this issue Apr 8, 2017 · 19 comments
Assignees

Comments

@steveri
Copy link
Contributor

steveri commented Apr 8, 2017

When Jeff changes the name of the const from "const" to "const_186" the flow breaks. I (steveri) added a hack in the travis script to undo the name, but this is of course not the right fix. I suspect the problem is either in the mapper or in one of the pnr scripts, respectively. You can see source for working and non-working design_top.json in builds 145 (https://travis-ci.org/StanfordAHA/CGRAFlow/builds/219574814) and 146 (https://travis-ci.org/StanfordAHA/CGRAFlow/builds/219898992) respectively---the only difference is one line where the working version says "const" and the nonworking version says "const_186"

diff design_top_145.json design_top_146.json
< "const"

"const_186"

@jeffsetter
Copy link
Contributor

Note that "const_186" is simply a name of the constant being using. Ideally subsequent scripts would not rely on the instance name for parsing.

@steveri
Copy link
Contributor Author

steveri commented Apr 9, 2017 via email

@jameshegarty
Copy link
Member

Remember Ross wrote the mapper, not me... I looked at his code, and he does hardcode a "const" in a few places, but it wasn't obvious to me that that was the problem. It could be. IMO @rdaly525 and @cdonovick should update their code to error out if the input doesn't match what is expected. As of right now, it looks like their code is silently failing, which makes this hard to track down.

I would maybe also recommend removing the workaround... keeping the build working is a good idea, but IMO this is a real problem we should track down. But thanks for doing this Steven!

Is everyone getting these alerts?

@jameshegarty
Copy link
Member

It's possible btw that this has nothing to do with Jeff's change, remember, we pull down all the repos each time so it could have been another change.

@steveri
Copy link
Contributor Author

steveri commented Apr 9, 2017 via email

@cdonovick
Copy link
Contributor

cdonovick commented Apr 9, 2017 via email

@steveri
Copy link
Contributor Author

steveri commented Apr 9, 2017 via email

@steveri
Copy link
Contributor Author

steveri commented Apr 9, 2017 via email

@cdonovick
Copy link
Contributor

cdonovick commented Apr 9, 2017 via email

@cdonovick
Copy link
Contributor

cdonovick commented Apr 9, 2017 via email

@steveri
Copy link
Contributor Author

steveri commented Apr 9, 2017 via email

@steveri
Copy link
Contributor Author

steveri commented Apr 9, 2017 via email

@steveri
Copy link
Contributor Author

steveri commented Apr 9, 2017 via email

@steveri
Copy link
Contributor Author

steveri commented Apr 9, 2017 via email

@steveri
Copy link
Contributor Author

steveri commented Apr 10, 2017

As recommended earlier by James, I am going to remove my sed hack from the travis script so that it will fail until we get this fixed...

I would maybe also recommend removing the workaround... keeping the build working is a good idea, but IMO this is a real problem we should track down.

@steveri
Copy link
Contributor Author

steveri commented Apr 10, 2017

BTW if (like me) you are annoyed that everyone can see your comments except you, note that there is a place in your account settings where you can set "include your own updates" (seems to be off by default) - see https://github.com/settings/notifications

@rdaly525
Copy link
Contributor

I am going to push a change that should fix this problem, but break Jeff's code.
@jeffsetter I am changing the type of constant, so please update your code to take this into account.
Previous: Array(16,BitOut)
New: Record({"out",Array(16,BitOut)})

@jeffsetter
Copy link
Contributor

I pushed the necessary change to my repo. It appears that it is working again.

@jameshegarty
Copy link
Member

Great!

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

No branches or pull requests

5 participants