Skip to content

Raise Error when HTTPReader get 404 Response (#160)#569

Closed
diegoaichele wants to merge 6 commits intometa-pytorch:mainfrom
diegoaichele:main
Closed

Raise Error when HTTPReader get 404 Response (#160)#569
diegoaichele wants to merge 6 commits intometa-pytorch:mainfrom
diegoaichele:main

Conversation

@diegoaichele
Copy link
Contributor

@diegoaichele diegoaichele commented Jul 4, 2022

Fixes #160

Changes

  • Add line of requests raise_for_status() will only allow 200 accepted requests

Comments

  • This raise only run when the method iter of HTTPReader is called.
  • It is not only raise for the 503 error, it also runs for 404, 500,501,502 ...
  • I simulate them using FastAPI with the next code.
from fastapi import FastAPI, HTTPException

app = FastAPI()

@app.get("/raise/{error_int}")
async def raise_error(error_int: int):
    raise HTTPException(status_code=error_int)

@diegoaichele
fixissue160: Raise Error when HTTPReader get 503 Response
746a2c5 
- Add line of requests raise_for_status() will only allow 200 accepted requests
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 4, 2022
@msaroufim msaroufim self-requested a review July 4, 2022 23:25
@msaroufim msaroufim self-requested a review July 5, 2022 18:11
VitalyFedyunin
VitalyFedyunin reviewed Jul 6, 2022
View reviewed changes
Copy link
Contributor

@VitalyFedyunin VitalyFedyunin left a comment

Choose a reason for hiding this comment

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

Can you please add a test to https://github.com/pytorch/data/blob/main/test/test_remote_io.py#L60 which tries to load an unexisting url?

@diegoaichele
Merge branch 'main' into main
8c430b8
@diegoaichele
feat: Add Test HTTPError exception on the test_http_reader_iterdatapipe
6b5edc3 
shorten the error message ([:54]), but the important thing is that the 404 response comes out
@diegoaichele
Copy link
Contributor Author

diegoaichele commented Jul 19, 2022

@VitalyFedyunin
feat: Add Test HTTPError exception on the test_http_reader_iterdatapipe
shorten the error message ([:54]), but the important thing is that the 404 response comes out

VitalyFedyunin
VitalyFedyunin reviewed Jul 20, 2022
View reviewed changes
test/test_remote_io.py Outdated
# Error Test: test if the Http Reader raises an error when the url is invalid
error_url = "https://github.com/pytorch/data/this/url/dont/exist"
http_error_dp = HttpReader(IterableWrapper([error_url]), timeout=timeout)
with self.assertRaises(Exception) as e:
Copy link
Contributor

@VitalyFedyunin VitalyFedyunin Jul 20, 2022

Choose a reason for hiding this comment

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

Please use assertRaisesRegex it will reduce lines 91-92 to simple check.

@VitalyFedyunin VitalyFedyunin changed the title Raise Error when HTTPReader get 503 Response (#160) Raise Error when HTTPReader get 404 Response (#160) Jul 20, 2022
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 20, 2022

@NivekT has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 21, 2022

@NivekT has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@NivekT NivekT added ciflow/slow Run tests with decorate of `slowTest` ciflow/period Run period tests labels Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@msaroufim msaroufim Awaiting requested review from msaroufim

@VitalyFedyunin VitalyFedyunin Awaiting requested review from VitalyFedyunin

1 more reviewer

@NivekT NivekT NivekT approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ciflow/period Run period tests ciflow/slow Run tests with decorate of `slowTest` CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Raise Error when HTTPReader get 503 Response

5 participants

@diegoaichele @facebook-github-bot @VitalyFedyunin @msaroufim @NivekT