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

Upgrade all packages using pur tool #2916

Merged
merged 10 commits into from
Mar 15, 2018
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented May 28, 2017

These ones were not upgraded since they are incompatible:

  • django
  • docker-py
  • celery
  • elasticsearch
  • pyelasticsearch

This is just a way to run all the test with this environment and also to discuss with other people if this is something we really want.

Closes #1893

@humitos humitos added the PR: work in progress Pull request is not ready for full review label May 28, 2017
@humitos
Copy link
Member Author

humitos commented Nov 16, 2017

@agjohnson is this something that we want or do we prefer to upgrade packages one by one? Let me know, so I can update the PR or just close it.

@humitos humitos force-pushed the humitos/pip/upgrade-packages branch from 416ab40 to 611f6ac Compare December 21, 2017 01:23
@humitos
Copy link
Member Author

humitos commented Dec 21, 2017

Updated today to the latest versions compatible with our codebase.

@ericholscher @agjohnson please, let me know if you are interested on these kind of upgrade or not.

@humitos humitos added PR: ready for review and removed PR: work in progress Pull request is not ready for full review labels Dec 21, 2017
@ericholscher
Copy link
Member

I think it's a good practice. I will let this sit until Friday then merge it, so we can do some local testing etc on it before deploy.

@humitos
Copy link
Member Author

humitos commented Dec 23, 2017

It's Friday night here, nobody is around and Xmas is comming this weekend. So, maybe it's a better idea to merge this next week :)

@humitos humitos self-assigned this Jan 15, 2018
mkdocs==0.14.0
django==1.9.12
six==1.10.0
mkdocs==0.17.2
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't support mkdocs==0.17, not sure if we want this here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not? Do we you have an example where it fails?

I could write a test to make it fail so we don't hit this issue again and downgrade to 0.14.0 as it was before this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

mkdocs 0.17 doesn't work at all with RTD. They removed a feature we required. 0.16.x was the last working version

Copy link
Member Author

Choose a reason for hiding this comment

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

This is confusing for me.

First, why do we need mkdocs for RTD itself? In fact, can we remove it from here? This is the requirements for the RTD project not the mkdocs installed by default in the user builds.

I did a quick grep and I found that we have 0.14.0 in the RTD reqs and 0.15.0 in the reqs for user builds:

$ rg mkdocs=
requirements/pip.txt
9:mkdocs==0.14.0

readthedocs/doc_builder/python_environments.py
215:            requirements.append('mkdocs==0.15.0')

Copy link
Member Author

Choose a reason for hiding this comment

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

I pinned to 0.15.0 --the same that we install by default in the venvs

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know which feature was removed in 0.17? There's at least one ticket (#3991) where the recommended fix from an mkdocs contributor was to upgrade.

@humitos humitos force-pushed the humitos/pip/upgrade-packages branch from 7d957d7 to 5b18aac Compare January 24, 2018 21:47
@humitos
Copy link
Member Author

humitos commented Jan 24, 2018

@agjohnson I updated this PR with the latest packages

@humitos
Copy link
Member Author

humitos commented Jan 27, 2018

I didn't touch the deploy.txt requeriments, but just in case we want to upgrade it, here is the output:

$ pur --skip django-tastypie,django,docker-py,elasticsearch,pyelasticsearch,commonmark,stripe,djangorestframework -r requirements/deploy.txt 
New version for readthedocs-build found (2.0.9), but current spec prohibits updating: readthedocs-build<2.1
Updated gunicorn: 19.1.0 -> 19.7.1
Updated pysolr: 2.0.13 -> 3.7.0
Updated django-redis-cache: 1.6.3 -> 1.7.1
Updated django-mailgun: 0.2.2 -> 0.9.1
Updated hiredis: 0.1.2 -> 0.2.0
Updated cssselect: 0.7.1 -> 1.0.3
All requirements up-to-date.

Should I commit this changes? Is it safe?

This was referenced Feb 21, 2018
@xrmx
Copy link
Contributor

xrmx commented Feb 22, 2018

You may want to upgrade to linaro_django_pagination==2.0.3 as seen on #3305 too

@humitos
Copy link
Member Author

humitos commented Feb 22, 2018

@xrmx I didn't upgrade it because it pinned to a very specific commit from a fork of the original repo (I tried upgrading it and test failed) --and as I don't know the main reason I prefered to leave it as it was.

In fact, in the issue you linked here, they are talking about some problems with that package and they don't know how to fix it.

So, I wouldn't touch it without knowing the implicances. Maybe, as they are saying is good to remove that dependency after a research on how it would affect.

@humitos humitos force-pushed the humitos/pip/upgrade-packages branch from c46bdb0 to 814a20d Compare February 22, 2018 14:39
@humitos
Copy link
Member Author

humitos commented Feb 23, 2018

Tow more related PRs that I wanted to include in this branch:

@humitos
Copy link
Member Author

humitos commented Feb 23, 2018

@xrmx maybe you are interested in my last two PRs from my last comment :)

@xrmx
Copy link
Contributor

xrmx commented Feb 27, 2018

@humitos nice, thanks!

@xrmx
Copy link
Contributor

xrmx commented Feb 27, 2018

Wondering if pyelasticsearch can be dropped, git grep pyelasticsearch returns nothing and (current) haystack uses elasticsearch-py.

@humitos
Copy link
Member Author

humitos commented Feb 28, 2018

@xrmx maybe. How elasticsearch works currently in production is a mistery for me, so I didn't want to get involved on that. As I don't have experience with that I didn't touch it.

@xrmx
Copy link
Contributor

xrmx commented Feb 28, 2018

@humitos if nobody uses it, what can it break? Anyway i've removed it from my local installation and nothing changed.

@humitos
Copy link
Member Author

humitos commented Feb 28, 2018

@xrmx I'm not saying that nobody uses it. I'm saying that I don't know how it's used, if it is and that's why I didn't wanted to remove it.

The search engine is complex (in fact, you opened another issue saying that it doesn't work your local instance). So, you don't know if it doesn't break things.

@agjohnson
Copy link
Contributor

elasticsearch is indeed used, it shouldn't be removed.

This has a high breakage potential. We should plan on merging on a week where we have plenty of eyes on the deploy. I don't have any strong feedback here otherwise, so effectively +1. Perhaps next week we can merge early, QA, then deploy midweek?

@humitos
Copy link
Member Author

humitos commented Mar 8, 2018

@agjohnson agree with you. Let me know what to do with the mkdocs package, and we will be ready to go.

@xrmx
Copy link
Contributor

xrmx commented Mar 8, 2018

@agjohnson we were talking about pyelasticsearch not elasticsearch :)

@ericholscher
Copy link
Member

@humitos we have mkdocs there so that people running it locally can build mkdocs docs. This is less important now that we have docker support. There is likely a lot that could be removed that is a build dependency, but not required for basic dev.

@humitos
Copy link
Member Author

humitos commented Mar 9, 2018

@ericholscher thanks for the explanation about this. Considering what you have said, I think it's a good idea to split our requirements/pip.txt to have all the RTD specific requirements and another one for local builds (using LocalBuildEnvironment class). So, if you are running Docker you don't need to install them.

Besides, it easier to know what dependency is for. What do you think?

I will downgrade mkdocs to keep it compatible as @agjohnson said and if you like my proposal, I will achieve that in another PR.

@ericholscher
Copy link
Member

Big +1 on simplifying the requirements file for local dev. It should def. be in another PR, but I think it is a big reason that people have a hard time getting started, so reducing the "time to running local instance" is a good metric to increase contribution!

humitos added 8 commits March 14, 2018 18:44
These ones were not upgraded since they are incompatible:

* docker-py
* celery
* elasticsearch
* pyelasticsearch
    pur --skip django,docker-py,elasticsearch,pyelasticsearch

and some packages pinned manually to avoid conflicts with our tests.
In 6.x changelog

> DocParser has been renamed to Parser.

https://github.com/rtfd/CommonMark-py/releases/tag/0.6.0

and that it's not compatible with the latest version of
recommonmark (0.4.0)
It doesn't receive arguments and can't be used at that point because
it was called previously by the other POST on the API:

https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.assert_not_called
$ pur --skip django-tastypie,django,docker-py,elasticsearch,pyelasticsearch,commonmark,stripe,djangorestframework
django-allauth 0.33.0 doesn't support Django 1.9.x which is the
version we are using at the moment.
This version is the one installed by default on the venv to build the
user's documentation.
@agjohnson agjohnson force-pushed the humitos/pip/upgrade-packages branch from 5002a10 to 3ce5a4c Compare March 15, 2018 00:45
@agjohnson
Copy link
Contributor

Rebased this, should hopefully be able to be merged after tests.

Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

Mocks and random methods names...

`Project.superprojects.all()` returns `ProjectRelationship` instances
where we need to use the `parent` or `child` attribute to access to
the project itself.

(we were sending the `ProjectRelationship.pk`)
@humitos
Copy link
Member Author

humitos commented Mar 15, 2018

I think we are all good now.

Some context:

  • mock==1.0 probably wasn't checking that the assert_ method exist in the mock object and was considering it as another mocked method (arbitrary) that you can call --that's why it didn't fail in the test.
  • now, mock==2.0 is checking this and the test start failing because the name with the final s was a mistake
  • finally, there was a bug on how the broadcast method was being called inside the Project.save since it was using the ProjectRelationship.pk instead of the proper pk from the superproject :)

@agjohnson
Copy link
Contributor

Nice! I wonder if this actually resolves some of the problems with symlinking that we noticed in the deploys.

I'll go ahead and merge this and the docker upgrade as well.

@agjohnson agjohnson merged commit 50e7d4b into master Mar 15, 2018
@agjohnson agjohnson deleted the humitos/pip/upgrade-packages branch March 15, 2018 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants