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

Do not sort already ordered data. #1237

Merged
merged 7 commits into from
Feb 20, 2020
Merged

Do not sort already ordered data. #1237

merged 7 commits into from
Feb 20, 2020

Conversation

litghost
Copy link
Contributor

No description provided.

mithro
mithro reviewed Feb 14, 2020
View reviewed changes
utils/xjson.py Outdated Show resolved Hide resolved
@litghost litghost requested a review from mithro February 14, 2020 19:12
@litghost
Copy link
Contributor Author

I think this PR fixes the bugs introduced by #1230 without losing stable outputs.

@litghost litghost mentioned this pull request Feb 14, 2020
Merged
@mithro mithro mentioned this pull request Feb 15, 2020
Closed
@mithro
Copy link
Contributor

mithro commented Feb 15, 2020

FYI - I have an alternative to this pull request for your review at #1243

@litghost
Do not sort already ordered data.
822a61e 
Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>
@litghost
Use sort key for sets.
26d8024 
Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>
@litghost
Add some sorting to JSON outputs.
89761c1 
Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>
@litghost litghost mentioned this pull request Feb 18, 2020
Merged
@litghost litghost force-pushed the dont_sort_ordered_data branch 2 times, most recently from 1cf4823 to 53ddf60 Compare February 18, 2020 18:44
@litghost
Use extract_numbers for sort keys to preserve previous DB output.
222eefc 
Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>
@litghost
Avoid race in 001 fuzzer.
0364315 
Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>
@litghost
Remove broken special case logic from xjson.
c2a5c7f 
Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>
@litghost
Copy link
Contributor Author

litghost commented Feb 19, 2020 •
edited by unfurl-links bot

@mithro, I believe after many iterations this is ready for review. CI should be done in 1-2 hours, but to review everything except tileconn, you can take a look at https://source.cloud.google.com/results/invocations/cdf8a590-0ecc-4b5c-9a39-25c8377b493c

Develop your code on the Google Cloud Platform.

@litghost
Sort wire pairs using extract_numbers.
9aec0c8 
Signed-off-by: Keith Rothman <537074+litghost@users.noreply.github.com>
@litghost
Copy link
Contributor Author

litghost commented Feb 19, 2020 •
edited by unfurl-links bot

@mithro, I believe after many iterations this is ready for review. CI should be done in 1-2 hours, but to review everything except tileconn, you can take a look at https://source.cloud.google.com/results/invocations/cdf8a590-0ecc-4b5c-9a39-25c8377b493c

Develop your code on the Google Cloud Platform.

Artix7 run is complete and green, take a look and lets get this merged!

Develop your code on the Google Cloud Platform.

mithro
mithro reviewed Feb 19, 2020
View reviewed changes
Copy link
Contributor

@mithro mithro left a comment

Choose a reason for hiding this comment

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

Bunch of minor comments...

fuzzers/074-dump_all/generate_grid.py

tileconn = tuple(
sorted(
tileconn, key=lambda x: (x['tile_types'], x['grid_deltas'])))
Copy link
Contributor

Choose a reason for hiding this comment

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

You want to run extract_numbers(x['tule_types']) too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tile types doesn't tend to have many numbers, so I don't think it is required. Also it would required fixing the extract_numbers formatting. So maybe leave as follow on?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm, I think I was thinking about the site_types where I wanted the first site to be the first value....

Makefile
@@ -182,6 +180,8 @@ db-extras-artix7-harness:
+source minitests/roi_harness/arty-swbut.sh && \
$(MAKE) -C minitests/roi_harness \
HARNESS_DIR=$(XRAY_DATABASE_DIR)/artix7/harness/arty-a7/swbut run copy
+XRAY_PIN_00=J13 XRAY_PIN_01=J14 XRAY_PIN_02=K15 XRAY_PIN_03=K16 \
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit unsure why did this moved? A comment on what /exactly/ this is doing would be good. Should it be run before the harness steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This moved because there was a race condition in the previous location. #1227 happens to fix the race condition, so I wouldn't worry about it too much right now.

prjxray/xjson.py
if m.group(1):
bits.append(m.group(1))
if m.group(2):
bits.append(int(m.group(2)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you potentially want to convert the number back into a string with leading zeros.

Otherwise, extract_numbers("A100") > extract_numbers("100B") will complain about non-matching data types?

In [1]: (100, 't') > ('t', 100)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-d06ca7865f0c> in <module>()
----> 1 (100, 't') > ('t', 100)

TypeError: '>' not supported between instances of 'int' and 'str'

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's been stable for now, I think we can leave that for a future enhancement. In general when we do the extract_numbers, there is an underlying structure that tends to be uniform. A follow up with some unit tests and leading zeros would handle this. I'll make an issue for fix up.

prjxray/xjson.py
elif isinstance(o, str):
return extract_numbers(o)
elif isinstance(o, int):
return o
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you potentially want to convert the number back into a string with leading zeros.

prjxray/xjson.py
@@ -0,0 +1,98 @@
import io
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should rename this to something like prjxray/sort_complex_data.py or something now? The name xjson was just me being lazy I think....

@litghost litghost mentioned this pull request Feb 19, 2020
Open
@litghost
Copy link
Contributor Author

For now I'd like to leave further refactoring of prjxray/xjson as follow up work. I've filed #1248 to issues identified in this PR.

@mithro
Copy link
Contributor

mithro commented Feb 19, 2020

For now I'd like to leave further refactoring of prjxray/xjson as follow up work. I've filed #1248 to issues identified in this PR.

I'm okay with that.

@litghost litghost merged commit 9aec0c8 into f4pga:master Feb 20, 2020
@litghost litghost deleted the dont_sort_ordered_data branch October 23, 2020 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mithro mithro mithro left review comments

Assignees
No one assigned
Labels
parts-fuzzers parts-infra parts-minitests parts-tools
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@litghost @mithro @kokoro-team