Add URI support to tarball path and xclbin path #1432
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sifive-benjamin-morse
added
changelog:added
sifive-benjamin-morse
force-pushed
the
bmorse/uri
branch
2 times, most recently
from
462ca34
to
4d4eb42
Compare
use fsspec to enable xclbin's to be one of any URI protocol supported by the library or an installed add-on
it is the easy and safe thing to do as a first implementation. can revisit FileCache later
refactor `localize_xclbin` from the Vitis runner to `util.io.downloadURI()` and add some pytests
Co-authored-by: Filip Stamenkovic <92741622+filipstamenkovic-sifive@users.noreply.github.com>
awstools.py now depends on it via util.io
Co-authored-by: Abraham Gonzalez <abe.j.gonza@gmail.com>
* throw better exceptions when URI don't exist * remove forced check upgront for file:// URI to exist as this will be annoying when any neighbor hwdb entry could prevent runs * fix checking if server_hardware_config is a string or not * Please enter the commit message for your changes. Lines starting
sifive-benjamin-morse
requested review from
timsnyder-siv and
davidbiancolin
|
Uncaught 2 FPGA instance shutdown(s) detected for CI run: 4296740217. Verify CI state before submitting PR. |
…t_xclbin_filename, improve error handling around xbutil JSON split stderr/stdout
timsnyder-siv
approved these changes
Mar 1, 2023
There was a problem hiding this comment.
Thanks again for getting this really implemented @sifive-benjamin-morse
deploy/util/io.py
Outdated
| for attempt in range(retries): | ||
| assert tries > 0, "tries argument must be larger than 0" | ||
| for attempt in range(tries): | ||
| rootLogger.debug(f"Download attempt {attempt+1} of {tries}: '{uri}' to '{lpath}'") | ||
| try: | ||
| fs, rpath = url_to_fs(uri) |
There was a problem hiding this comment.
Are you sure you want the url_to_fs() call in the try block? It seems like exceptions arising here may be indicative of a larger issue that shouldn't be retried?
There was a problem hiding this comment.
For background, these two runs:
- https://github.com/firesim/firesim/actions/runs/4301469200/jobs/7498730505
- https://github.com/firesim/firesim/actions/runs/4300595364/jobs/7496935845
fail for two different reasons, but on the same line:
aiohttp.client_exceptions.ClientConnectorError: Cannot connect to host firesim-ci-vitis-xclbins.s3.***.amazonaws.com:443 ssl:default [Name or service not known]aiohttp.client_exceptions.ClientConnectorError: Cannot connect to host firesim-ci-vitis-xclbins.s3.***.amazonaws.com:443 ssl:default [Temporary failure in name resolution]
I added url_to_fs() inside the block because I wasn't sure if it did the DNS resolution or not. I will move it out. Thanks for reviewing this
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
config_hwdb.yamlto use URI's for thedriver_tarandxclbinkeys.server_hardware_configbeing a string.xclbin(such asrun-linux-poweroff-vitis.py) can now directly specify the URL as a URIRelated PRs / Issues
UI / API Impact
Allows much easier collaboration with new developers as pre-compiled resources can be put on the web simplifying first time runs.
Verilog / AGFI Compatibility
Should be no change.
Contributor Checklist
changelog:<topic>label?ci:fpga-deploylabel?Please Backportlabel?Reviewer Checklist (only modified by reviewer)
Note: to run CI on PRs from forks, comment
@Mergifyio copy mainand manage the change from the new PR.changelog:<topic>label?