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

Update Dependencies #196

Merged
merged 12 commits into from
Nov 26, 2018
Merged

Update Dependencies #196

merged 12 commits into from
Nov 26, 2018

Conversation

MarkKoz
Copy link
Member

@MarkKoz MarkKoz commented Nov 17, 2018

Closes #99.

  • Renamed discord to discord-py.
  • discord-py was changed to a git dependency.
  • Removed multidict and sympy from Pipfile because they weren't explicitly being used anywhere.
  • Removed websockets and yarl from Pipfile because they were only there to circumvent previous dependency issues with discord.py.
  • Moved requests from dev packages to "normal" packages because the docs cog imports it.
  • flake8 was updated to 3.6 and all new lint errors were fixed.

Things seem to still work fine, though I did not rigorously test every single command in the bot. One thing I couldn't test is the eval command. Also, I had trouble adding new docs (!docs set). However, both of these issues were present for me before these changes.

@lemonsaurus
Copy link
Member

I'm still not sure entirely why were are using requests instead of aiohttp which we use for all web requests in the rest of the bot. Feels like we should only need one of them. Any chance we can refactor some code to get rid of it altogether, or would that be a large enough job to warrant a separate PR?

@jb3
Copy link
Member

jb3 commented Nov 18, 2018

I'm pretty sure we are only using it for intersphinx and specifically an error which intersphinx might raise (iirc it's only imported once).

@jchristgit
Copy link
Member

Yeah, that's why. I should probably remove that somehow.

@jchristgit
Copy link
Member

Also, I had trouble adding new docs (!docs set).

What exactly went wrong there?

@MarkKoz
Copy link
Member Author

MarkKoz commented Nov 18, 2018

@jchristgit

!docs set discord https://discordpy.readthedocs.io/en/rewrite/ https://discordpy.readthedocs.io/en/rewrite/objects.inv

Nov 18 07:19:50 pd.beardfist.com Bot: |                   bot.cogs.doc |    ERROR | Unhandled error: Command raised an exception: HTTPError: ('intersphinx inventory %r not fetchable due to %s: %s', 'http://example.com/object.inv', <class 'requests.exceptions.HTTPError'>, HTTPError(...))
Traceback (most recent call last):
  File "C:\Repositories\Python\pydisc-bot\.venv\lib\site-packages\discord\ext\commands\core.py", line 61, in wrapped
    ret = await coro(*args, **kwargs)
  File "C:\Repositories\Python\pydisc-bot\bot\cogs\doc.py", line 442, in set_command
    await self.refresh_inventory()
  File "C:\Repositories\Python\pydisc-bot\bot\cogs\doc.py", line 184, in refresh_inventory
    await asyncio.gather(*coros)
  File "C:\Repositories\Python\pydisc-bot\bot\cogs\doc.py", line 153, in update_single
    for _, value in (await self.bot.loop.run_in_executor(None, fetch_func)).items():
  File "C:\Programs\Anaconda\Lib\concurrent\futures\thread.py", line 56, in run
    result = self.fn(*self.args, **self.kwargs)
  File "C:\Repositories\Python\pydisc-bot\.venv\lib\site-packages\sphinx\ext\intersphinx.py", line 181, in fetch_inventory
    f = _read_from_url(inv, config=app.config)
  File "C:\Repositories\Python\pydisc-bot\.venv\lib\site-packages\sphinx\ext\intersphinx.py", line 136, in _read_from_url
    r.raise_for_status()
  File "C:\Repositories\Python\pydisc-bot\.venv\lib\site-packages\requests\models.py", line 940, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: ('intersphinx inventory %r not fetchable due to %s: %s', 'http://example.com/object.inv', <class 'requests.exceptions.HTTPError'>, HTTPError(...))

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "C:\Repositories\Python\pydisc-bot\.venv\lib\site-packages\discord\ext\commands\bot.py", line 899, in invoke
    await ctx.command.invoke(ctx)
  File "C:\Repositories\Python\pydisc-bot\.venv\lib\site-packages\discord\ext\commands\core.py", line 1024, in invoke
    await ctx.invoked_subcommand.invoke(ctx)
  File "C:\Repositories\Python\pydisc-bot\.venv\lib\site-packages\discord\ext\commands\core.py", line 614, in invoke
    await injected(*ctx.args, **ctx.kwargs)
  File "C:\Repositories\Python\pydisc-bot\.venv\lib\site-packages\discord\ext\commands\core.py", line 70, in wrapped
    raise CommandInvokeError(e) from e
discord.ext.commands.errors.CommandInvokeError: Command raised an exception: HTTPError: ('intersphinx inventory %r not fetchable due to %s: %s', 'http://example.com/object.inv', <class 'requests.exceptions.HTTPError'>, HTTPError(...))

Like I said, this was happening before updating any dependencies so it's unrelated. I just wanted it to be clear I cannot properly test this command in case there is something else that would break once this is resolved.

@MarkKoz
Copy link
Member Author

MarkKoz commented Nov 18, 2018

@gdude2002 Were @scragly's comments regarding the git dependency addressed?

@jchristgit
Copy link
Member

@MarkKoz looks like you have some example entry in your local database with an invalid entry. can you remove that and retry?

@MarkKoz
Copy link
Member Author

MarkKoz commented Nov 18, 2018

@jchristgit Ah yeah, it was the illustrious lemonapi. The set command was working fine all along, that error was related to refreshing the inventory I guess.

@MarkKoz MarkKoz self-assigned this Nov 18, 2018
bot/cogs/eval.py Show resolved Hide resolved
bot/utils/snakes/hatching.py Show resolved Hide resolved
Pipfile Show resolved Hide resolved
GhostofGoes
GhostofGoes previously approved these changes Nov 19, 2018
@MarkKoz MarkKoz added the a: dependencies Related to package dependencies and management label Nov 19, 2018
@jchristgit jchristgit self-assigned this Nov 19, 2018
jchristgit
jchristgit previously approved these changes Nov 19, 2018
Copy link
Member

@jchristgit jchristgit left a comment

Choose a reason for hiding this comment

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

Looks good.

bot/cogs/eval.py Show resolved Hide resolved
@MarkKoz
Copy link
Member Author

MarkKoz commented Nov 19, 2018

There's still some discussion points that need to be resolved:

  1. Can a git URL be used for discord.py?
  2. Can we get rid of requests?

@jchristgit
Copy link
Member

Can a git URL be used for discord.py?

Why not?

Can we get rid of requests?

I'll take care of requests in a separate PR; I have a few issues with setting up the bot locally currently.

@MarkKoz
Copy link
Member Author

MarkKoz commented Nov 19, 2018

@gdude2002 had something against it though he hasn't gotten back to me on that.

@jb3
Copy link
Member

jb3 commented Nov 19, 2018

Regarding the git dependency, @scragly made a good point yesterday in the dev-contrib channel about how any breaking change to the discord.py API (I believe cogs was used as an example) will cause us to have problems when we start auto-deploying the new version.

AFAIK there is no way to pin to a specific ref with a zip. I think that the risk of deploying a breaking change is not one we should be taking our chances with.

@jb3
Copy link
Member

jb3 commented Nov 23, 2018

I'm in favor of switching out for a git dependency. I think the issue about it being "harder for beginners" is not a really valid thing here. Most of our contributors will already have git installed (to clone the repository) and there was also a mention of it making web editing harder (?) which wouldn't really be valid because you don't download dependencies in the GitHub web editor.

@python-discord/staff if any of y'all have an opinion which is against adding a git dependency please state it now.

@MarkKoz MarkKoz dismissed stale reviews from jchristgit and GhostofGoes via 957ebe3 November 23, 2018 20:08
@MarkKoz
Copy link
Member Author

MarkKoz commented Nov 23, 2018

Build is failing because it's now linting discord.py's source. It's in .venv/src/discord-py. tox.ini already excludes .venv.

For what it's worth, running the linter locally yields no errors.

@jb3
Copy link
Member

jb3 commented Nov 24, 2018

@gdude2002 any idea why linting is trying to lint the .venv here? Is Azure not seeing the tox.ini file or something? was found to be related to an env var being passed into pipenv.

@gdude2002
Copy link
Contributor

It isn't linting .venv or anything in it. It's linting in ./src.

@MarkKoz
Copy link
Member Author

MarkKoz commented Nov 25, 2018

We can either specify the directory to checkout editable dependencies into with --src option for pipenv (or with PIP_SRC env var?) or we can just exclude src in tox.ini.

@gdude2002
Copy link
Contributor

We could use PIP_SRC and put it in the cache dir somewhere?

@MarkKoz
Copy link
Member Author

MarkKoz commented Nov 25, 2018

What is the cache directory? /opt/hostedtoolcache/? .cache/pipenv?

Copy link
Contributor

@gdude2002 gdude2002 left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@lemonsaurus lemonsaurus left a comment

Choose a reason for hiding this comment

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

looks good to this guy

@lemonsaurus lemonsaurus merged commit 36f6952 into master Nov 26, 2018
@MarkKoz MarkKoz deleted the issue/99-update-pipfile branch December 23, 2018 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: dependencies Related to package dependencies and management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Pipfile.lock
6 participants