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

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

Closed
wants to merge 6 commits into from

Conversation

diegoaichele
Copy link
Contributor

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)

- 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
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?

shorten the error message ([:54]), but the important thing is that the 404 response comes out
@diegoaichele
Copy link
Contributor Author

@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

# 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

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

@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

@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
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
Development

Successfully merging this pull request may close these issues.

Raise Error when HTTPReader get 503 Response
5 participants