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.

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

vim-plugins: Add --debug flag to update.py. #83008

Closed
wants to merge 1 commit into from
Closed

vim-plugins: Add --debug flag to update.py. #83008

wants to merge 1 commit into from

Conversation

ryneeverett
Copy link
Contributor

Motivation for this change

I don't know how anybody is able to use this pooling so many github requests simultaneously. Github has always rate-banned me for doing stuff like this, even synchronously if it's too fast. At least with this flag one doesn't have to edit update.py to toggle this behavior.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

rvolosatovs
rvolosatovs reviewed Mar 27, 2020
View reviewed changes
pkgs/misc/vim-plugins/update.py Outdated
@@ -407,6 +407,12 @@ def parse_args():
default=DEFAULT_OUT,
help="Filename to save generated nix code",
)
parser.add_argument(
"--debug",
Copy link
Member

Choose a reason for hiding this comment

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

How about --sync ?

Copy link
Member

Choose a reason for hiding this comment

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

You can mention in the help test that this is useful for debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Mic92
Copy link
Member

Mic92 commented Mar 27, 2020

I am surprised that you are getting blocked. 30 requests is not really excessive. Most browsers open up to 30 requests in parallel. I never saw this issue.

@rvolosatovs
Copy link
Member

I am surprised that you are getting blocked. 30 requests is not really excessive. Most browsers open up to 30 requests in parallel. I never saw this issue.

Me neither, however I just tried it myself and got this(which is how I stumbled upon this issue in the first place):

HTTP Error 429: too many requests, Retrying in 3 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...
HTTP Error 429: too many requests, Retrying in 6 seconds...

@Mic92
Copy link
Member

Mic92 commented Mar 27, 2020

Ok. Maybe they changed something in their load balancer lately. In that case we maybe should try to reduce the pool size.

@Mic92
Copy link
Member

Mic92 commented Mar 27, 2020

It seems even a bit more trickier than that. They block not concurrent requests but something like requests per minute. In that case it depends on the users network connection when the throttling kicks in.

@Mic92
Copy link
Member

Mic92 commented Mar 27, 2020

An alternative would be the use of API keys. Than we have something like 5000 requests.

@Mic92
Copy link
Member

Mic92 commented Mar 27, 2020

This might also work to get the latest commit:

git ls-remote -h https://github.com/NixOS/nixpkgs.git master
1cfd2436e01c347fec26fce0937364ebb51c1e2f        refs/heads/master

I don't know though if they also apply the same rate limits on their git api.

@ryneeverett
Copy link
Contributor Author

I am surprised that you are getting blocked. 30 requests is not really excessive. Most browsers open up to 30 requests in parallel.

Seems like most major services only trigger rate-banning if javascript is not being executed. I've been rate-banned from github for navigating too quickly in the browser with javascript disabled.

@teto
Copy link
Member

teto commented Mar 27, 2020

Never had any problem with update.py. I just tried to package 3 vim plugins and witnessed the "new" limitation. Wouldn't it be more adaptable to choose the number of processes ? rather than a boolean

@ryneeverett
Copy link
Contributor Author

@teto Yes, that would be more adaptable to this particular use case, but there is already a use case for updating synchronously, it was just commented out rather than exposed via flag.

@teto
Copy link
Member

teto commented Mar 27, 2020

Don't you achieve the same behavior when setting processes to 1 ? this is less code.
The flag could just change the default of 30 to 1, though I think it might be safer to globally default to 1 (better have it working and let people look into how to speedup).

@ryneeverett
Copy link
Contributor Author

@teto You're right. I was under the impression that pdb wouldn't work but it does.

I'd be fine with eliminating --debug/--sync altogether and just having a --proc option taking an integer. Any objections? @Mic92 @rvolosatovs

@rvolosatovs
Copy link
Member

@teto You're right. I was under the impression that pdb wouldn't work but it does.

I'd be fine with eliminating --debug/--sync altogether and just having a --proc option taking an integer. Any objections? @Mic92 @rvolosatovs

Sounds good to me!

@teto
Copy link
Member

teto commented Mar 28, 2020

Unrelated probably but even after setting the number of processes to 1, I can't update from where I am (I never had these in the past):

HTTP Error 502: Bad Gateway, Retrying in 6 seconds...
HTTP Error 502: Bad Gateway, Retrying in 12 seconds...

ryneeverett added a commit to ryneeverett/nixpkgs that referenced this pull request Mar 30, 2020
@ryneeverett
vimPlugins: Add --proc flag to update script.
9546a64 
This is based on discussion in NixOS#83008 and replaces it.
@ryneeverett ryneeverett mentioned this pull request Mar 30, 2020
Merged
10 tasks
@ryneeverett
Copy link
Contributor Author

Closing in favor of #83798.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@rvolosatovs rvolosatovs rvolosatovs left review comments

@jonringer jonringer Awaiting requested review from jonringer

Assignees
No one assigned
Labels
6.topic: vim 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ryneeverett @Mic92 @rvolosatovs @teto