Skip to content

[FBcode->GH] remove unused requests functionality #5014

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
Merged
merged 2 commits into from
Nov 30, 2021
Merged

[FBcode->GH] remove unused requests functionality #5014

merged 2 commits into from
Nov 30, 2021

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Nov 30, 2021 •
edited by pytorch-probot bot
Loading

Addresses #5013 (review) and supersedes #5013.

cc @pmeier

@facebook-github-bot
Copy link

facebook-github-bot commented Nov 30, 2021 •
edited
Loading

💊 CI failures summary and remediations

As of commit 42b8e7d (more details on the Dr. CI page):

✅ None of the CI failures appear to be your fault 💚

🚧 1 ongoing upstream failure:

These were probably caused by upstream breakages that are not fixed yet.

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@pmeier
Copy link
Collaborator Author

pmeier commented Nov 30, 2021

It seems, we make the change in #5013. @prabhat00155 feel free to close or merge this what ever is easier for you.

datumbox
datumbox reviewed Nov 30, 2021
View reviewed changes
Copy link
Contributor

@datumbox datumbox left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I would prefer to merge this (see #5013 (comment)) but I'm not going to approve to avoid conflicts and accidental merges. I'll leave @prabhat00155 to decide the course of action.

prabhat00155
prabhat00155 approved these changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@prabhat00155 prabhat00155 left a comment

Choose a reason for hiding this comment

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

I have made these changes internally to fix the linter error. So, we should not sync these changes again. I am happy to merge this PR(after adding FBcode->GH tag).

@prabhat00155 prabhat00155 changed the title remove unused requests functionality [FBcode->GH] remove unused requests functionality Nov 30, 2021
@prabhat00155 prabhat00155 merged commit 33123be into pytorch:main Nov 30, 2021
@datumbox
Copy link
Contributor

datumbox commented Dec 1, 2021

@prabhat00155 I had a look on the internal diffs to understand what you were talking about and why you were saying it was OK to sync the other PR. What I currently understand happened was:

  1. During your weekly sync one of the diffs had issues on FBcode and you had to make changes in-place to make it in.
  2. These changes where exported to Github by creating a new PR.

Your point was that either merging #5013 or #5014, it would be the same. And you are right.

What I understood was different. I thought someone else internally submitted a change on FBcode and you were trying to sync it on Github. Since we identified an issue with the fix, the usual course of action would be to merge as-is #5013 and open a new PR (say #5014) with the change which in turn would have to be synced back to FBcode.

I'm sorry for the misunderstanding, I confirm that the course of action you took is the correct and that this PR shouldn't be synced back to FBcode.

@pmeier pmeier deleted the completely-remove-requests branch December 1, 2021 09:16
pmeier added a commit to pmeier/vision that referenced this pull request Dec 7, 2021
@pmeier
Revert "[FBcode->GH] remove unused requests functionality (pytorch#5014…
e1d1962 
…)"

This reverts commit 33123be.
pmeier added a commit to pmeier/vision that referenced this pull request Dec 7, 2021
@pmeier
Revert "[FBcode->GH] remove unused requests functionality (pytorch#5014…
72049c4 
…)"

This reverts commit 33123be.
pmeier added a commit that referenced this pull request Dec 8, 2021
@pmeier
Revert removing requests and make it a hard dependency instead (#5047)
4282c9f 
* Revert "[FBcode->GH] remove unused requests functionality (#5014)"

This reverts commit 33123be.

* Revert "replace requests with urllib (#4973)"

This reverts commit 8d25de7.

* add requests as hard dependency

* install library stubs in CI

* fix syntax

* add requests to conda dependencies

* fix mypy CI
facebook-github-bot pushed a commit that referenced this pull request Dec 9, 2021
@facebook-github-bot
[fbsync] Revert removing requests and make it a hard dependency ins…
a733b37 
…tead (#5047)

Summary:
* Revert "[FBcode->GH] remove unused requests functionality (#5014)"

This reverts commit 33123be.

* Revert "replace requests with urllib (#4973)"

This reverts commit 8d25de7.

* add requests as hard dependency

* install library stubs in CI

* fix syntax

* add requests to conda dependencies

* fix mypy CI

Reviewed By: NicolasHug

Differential Revision: D32950928

fbshipit-source-id: de0ed9a1c7cb5ca6e1b2a30db76e3ccc2008d25c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@datumbox datumbox datumbox left review comments

@prabhat00155 prabhat00155 prabhat00155 approved these changes

Assignees
No one assigned
Labels
ciflow/default cla signed code quality dependency issue module: datasets
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pmeier @facebook-github-bot @datumbox @prabhat00155