add xclbin URI support #1140

timsnyder-siv · 2022-07-18T19:17:59Z

use fsspec to enable xclbin's to be one of any URI protocol
supported by the library or an installed add-on

Related PRs / Issues

UI / API Impact

Enables xclbin in hwdb to be a URI supported by fsspec

Verilog / AGFI Compatibility

No impact to Verilog or AGFI.

Contributor Checklist

Is this PR's title suitable for inclusion in the changelog and have you added a changelog:<topic> label?
Did you add Scaladoc/docstring/doxygen to every public function/method?
Update the docs with
- list of protocols tested and known to work
- pointer to fsspec config that can be used to inject auth stuff (TO ALL CALLS). Since the auth args go to all calls when using the fsspec level config, it won't work in all cases but it might be useful as we start to use things and determine if/how we want to add features to the manager to specify auth. In the limit, you might need different auth for every file you interact with but you don't want to have to repeat and maintain auth everywhere. Just brainstorming, it seems like fsspec should allow it's config to do URI pattern matching to provide auth.
Did you add at least one test demonstrating the PR?
Add a test for ssh:// in Add a test for ssh: fsspec URI #1167
Did you delete any extraneous prints/debugging code?
Did you state the UI / API impact?
Did you specify the Verilog / AGFI compatibility impact?

If applicable, did you regenerate and publicly share default AGFIs?

If applicable, did you apply the ci:fpga-deploy label?

If applicable, did you apply the Please Backport label?

Reviewer Checklist (only modified by reviewer)

Is the title suitable for inclusion in the changelog and does the PR have a changelog:<topic> label?
Did you mark the proper release milestone?
Did you check whether all relevant Contributor checkboxes have been checked?

use fsspec to enable xclbin's to be one of any URI protocol supported by the library or an installed add-on

timsnyder-siv · 2022-07-18T19:19:09Z

I haven't tested this at all yet but I wanted to get this PR open and share it with others to throw stones.

abejgonzalez

Seems pretty good to me so far minus some small nits.

deploy/runtools/runtime_config.py

docs/Advanced-Usage/Manager/Manager-Configuration-Files.rst

deploy/runtools/run_farm_deploy_managers.py

deploy/runtools/runtime_config.py

it is the easy and safe thing to do as a first implementation. can revisit FileCache later

timsnyder-siv · 2022-07-25T20:17:22Z

I have some more commits that I want to push to this after rebasing on top of the fixed pytests (because they weren't really broken by this stuff)

deploy/runtools/run_farm_deploy_managers.py

docs/Advanced-Usage/Manager/Manager-Configuration-Files.rst

timsnyder-siv · 2022-07-30T14:34:21Z

I updated the checklist with the remaining tasks for this PR to be ready to merge.

refactor `localize_xclbin` from the Vitis runner to `util.io.downloadURI()` and add some pytests

timsnyder-siv · 2022-08-05T15:03:41Z

deploy/tests/test_utils.py

+        ('s3',Path("tests/s3_test_download_json.json")),
+        ('file',Path("tests/file_test_download_json.json")),


Do we want to add a test for ssh? The simplest way to do it would be to spin up a docker container that the test would try to pull things out of with ssh. If docker isn't installed, the test would be skipped.

Alternately, we write a test for this that utilizes the multiple hosts in CI. Or we could do both so that devs can test this locally (if they have docker installed) and we have a test in CI without having to install docker in the place where we're running pytests.

Couldn't we just test this by running localhost for the "it should work" case and some invalid IP for the "it shouldn't work" case? I'm not sure it's worth having docker installed for such a small test

@abejgonzalez how do we differentiate whether someone doesn't have sshd configured correctly on localhost vs the test actually working? I don't like having a dependency into sshd that the tests aren't actually controling the configuration for.

A lighter-weight option could be for a test fixture to spin up a Paramiko sshd on a non-system port or use something like this gist. What do you think?

That seems better to me.

I've asked @filipstamenkovic-sifive to help me add a test.

Captured in #1167 for followup. I'd like to go ahead and get this merged if the CI passes this time.

filipstamenkovic-sifive

@timsnyder-siv adding a few suggestions to fix the test issue in CI.

deploy/tests/test_utils.py

scripts/machine-launch-script.sh

Co-authored-by: Filip Stamenkovic <[email protected]>

deploy/util/io.py

awstools.py now depends on it via util.io

Co-authored-by: Abraham Gonzalez <[email protected]>

abejgonzalez

LGTM. Pending all CI passing.

This reverts commit a517b19.

sifive-benjamin-morse · 2023-02-10T19:13:50Z

deploy/runtools/run_farm_deploy_managers.py

+            hwcfg.local_xclbin = './local.xclbin'
+
+            with cd(remote_sim_dir):
+                run(downloadURI, hwcfg.xclbin, hwcfg.local_xclbin)


From my testing, this line doesn't execute. run() only accepts a string, which is a shell command, not a function. The 2nd and 3rd arguments are booleans, not arguments to the first function. Am I on the wrong version of fabric or something? https://github.com/fabric/fabric/blob/1.14/fabric/operations.py#L970

I think that we meant execute() instead of run().

xclbin URI support

c20931e

use fsspec to enable xclbin's to be one of any URI protocol supported by the library or an installed add-on

timsnyder-siv added the changelog:added Put PR title in 'Added' section of changelog label Jul 18, 2022

timsnyder-siv requested review from sagark, davidbiancolin and abejgonzalez July 18, 2022 19:18

timsnyder-siv added the DONOTMERGE label Jul 18, 2022

abejgonzalez requested changes Jul 19, 2022

View reviewed changes

davidbiancolin reviewed Jul 21, 2022

View reviewed changes

deploy/runtools/run_farm_deploy_managers.py Show resolved Hide resolved

deploy/runtools/run_farm_deploy_managers.py Outdated Show resolved Hide resolved

deploy/runtools/run_farm_deploy_managers.py Outdated Show resolved Hide resolved

deploy/runtools/runtime_config.py Show resolved Hide resolved

timsnyder added 5 commits July 24, 2022 17:10

add helpful comment for sim_slot bounds check assertion

99bbbef

cleanup some formatting nits

da27c75

overwrite local files and download again in localize_xclbin()

9bd9bce

it is the easy and safe thing to do as a first implementation. can revisit FileCache later

add dep for fsspec

7e93452

cleanup mypy checks

057bfd4

Merge remote-tracking branch 'origin/main' into fsspec_for_bitstream

86ddfd8

timsnyder-siv commented Jul 26, 2022

View reviewed changes

deploy/runtools/run_farm_deploy_managers.py Show resolved Hide resolved

timsnyder-siv commented Jul 26, 2022

View reviewed changes

deploy/runtools/run_farm_deploy_managers.py Show resolved Hide resolved

cleanup rst typos

af2c451

timsnyder-siv commented Jul 26, 2022

View reviewed changes

docs/Advanced-Usage/Manager/Manager-Configuration-Files.rst Outdated Show resolved Hide resolved

timsnyder-siv mentioned this pull request Jul 27, 2022

Use of pathlib/universal_path #1148

Open

add pointer where the RFC-3986 regex came from (pandas)

b872a71

timsnyder-siv changed the title ~~xclbin URI support~~ add xclbin URI support Jul 30, 2022

timsnyder-siv mentioned this pull request Aug 4, 2022

add tests for downloadURI #1155

Merged

12 tasks

add tests for downloadURI (#1155)

80b6fe3

refactor `localize_xclbin` from the Vitis runner to `util.io.downloadURI()` and add some pytests

timsnyder-siv commented Aug 5, 2022

View reviewed changes

filipstamenkovic-sifive reviewed Aug 5, 2022

View reviewed changes

deploy/tests/test_utils.py Outdated Show resolved Hide resolved

scripts/machine-launch-script.sh Show resolved Hide resolved

timsnyder-siv and others added 2 commits August 5, 2022 16:08

Add region to test and s3fs to requirements

153ee4a

Co-authored-by: Filip Stamenkovic <[email protected]>

Merge remote-tracking branch 'origin/main' into fsspec_for_bitstream

b7fbb3d

abejgonzalez reviewed Aug 9, 2022

View reviewed changes

deploy/util/io.py Outdated Show resolved Hide resolved

fixup fsspec docs to point to fsspec config

2268d44

timsnyder-siv removed the DONOTMERGE label Aug 9, 2022

timsnyder-siv and others added 2 commits August 9, 2022 14:14

Add fsspec to CI requirements.txt

765948a

awstools.py now depends on it via util.io

Remove debug prints

3494daf

Co-authored-by: Abraham Gonzalez <[email protected]>

timsnyder-siv requested a review from abejgonzalez August 9, 2022 19:34

abejgonzalez approved these changes Aug 9, 2022

View reviewed changes

Remove StreamLogger that was still in run_farm_deploy_managers.

98af391

timsnyder-siv mentioned this pull request Aug 9, 2022

Add a test for ssh: fsspec URI #1167

Open

timsnyder-siv enabled auto-merge (squash) August 9, 2022 22:40

timsnyder-siv closed this Aug 10, 2022

auto-merge was automatically disabled August 10, 2022 14:30
Pull request was closed

timsnyder-siv reopened this Aug 10, 2022

timsnyder-siv merged commit a517b19 into main Aug 10, 2022

timsnyder-siv deleted the fsspec_for_bitstream branch August 10, 2022 21:00

filipstamenkovic-sifive mentioned this pull request Aug 11, 2022

Add a test for downloadURI for ssh protocol #1169

Closed

12 tasks

This was referenced Aug 11, 2022

Add a ssh: test for downloadURI #1170

Closed

run-basic-linux-poweroff-vitis failing because of new deps in machine-launch-script.sh #1171

Closed

timsnyder-siv added a commit that referenced this pull request Aug 16, 2022

Revert "add xclbin URI support (#1140)"

ad05b76

This reverts commit a517b19.

timsnyder-siv mentioned this pull request Aug 16, 2022

Revert "add xclbin URI support (#1140)" #1173

Merged

12 tasks

timsnyder-siv added changelog:omit Do not include this PR in the changelog and removed changelog:added Put PR title in 'Added' section of changelog labels Aug 16, 2022

timsnyder-siv added a commit that referenced this pull request Aug 16, 2022

Revert "add xclbin URI support (#1140)" (#1173)

8cb2325

This reverts commit a517b19.

abejgonzalez restored the fsspec_for_bitstream branch January 28, 2023 01:33

sifive-benjamin-morse reviewed Feb 10, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add xclbin URI support #1140

add xclbin URI support #1140

timsnyder-siv commented Jul 18, 2022 •

edited

Loading

timsnyder-siv commented Jul 18, 2022

abejgonzalez left a comment

timsnyder-siv commented Jul 25, 2022

timsnyder-siv commented Jul 30, 2022

timsnyder-siv Aug 5, 2022

abejgonzalez Aug 9, 2022

timsnyder-siv Aug 9, 2022

abejgonzalez Aug 9, 2022

timsnyder-siv Aug 9, 2022

timsnyder-siv Aug 9, 2022

filipstamenkovic-sifive left a comment

abejgonzalez left a comment

sifive-benjamin-morse Feb 10, 2023 •

edited

Loading

timsnyder-siv Feb 17, 2023

		('s3',Path("tests/s3_test_download_json.json")),
		('file',Path("tests/file_test_download_json.json")),

add xclbin URI support #1140

add xclbin URI support #1140

Conversation

timsnyder-siv commented Jul 18, 2022 • edited Loading

Related PRs / Issues

UI / API Impact

Verilog / AGFI Compatibility

Contributor Checklist

Reviewer Checklist (only modified by reviewer)

timsnyder-siv commented Jul 18, 2022

abejgonzalez left a comment

Choose a reason for hiding this comment

timsnyder-siv commented Jul 25, 2022

timsnyder-siv commented Jul 30, 2022

timsnyder-siv Aug 5, 2022

Choose a reason for hiding this comment

abejgonzalez Aug 9, 2022

Choose a reason for hiding this comment

timsnyder-siv Aug 9, 2022

Choose a reason for hiding this comment

abejgonzalez Aug 9, 2022

Choose a reason for hiding this comment

timsnyder-siv Aug 9, 2022

Choose a reason for hiding this comment

timsnyder-siv Aug 9, 2022

Choose a reason for hiding this comment

filipstamenkovic-sifive left a comment

Choose a reason for hiding this comment

abejgonzalez left a comment

Choose a reason for hiding this comment

sifive-benjamin-morse Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

timsnyder-siv Feb 17, 2023

Choose a reason for hiding this comment

timsnyder-siv commented Jul 18, 2022 •

edited

Loading

sifive-benjamin-morse Feb 10, 2023 •

edited

Loading