Move minimum requirements #1149

Merged
merged 7 commits into from Aug 6, 2013

Conversation

Projects
None yet
2 participants
Contributor

domoritz commented Aug 1, 2013

We need a place to store minimum requirements so that we know when a requirement of a direct requirement is removed.

Minimum requirements in requirements.in.

domoritz was assigned Aug 1, 2013

@domoritz domoritz added a commit that referenced this pull request Aug 1, 2013

@domoritz domoritz [#1149] Try future version of pip-tools ae1bedb

@domoritz domoritz added a commit that referenced this pull request Aug 1, 2013

@domoritz domoritz [#1149] First try with pip-compile 5b35011

@domoritz domoritz added a commit that referenced this pull request Aug 1, 2013

@domoritz domoritz [#1149] Fix duplicate requirements f64907b

@domoritz domoritz added a commit that referenced this pull request Aug 1, 2013

@domoritz domoritz [#1149] Fix requirements 63e728e

@domoritz domoritz added a commit that referenced this pull request Aug 1, 2013

@domoritz domoritz [#1149] Freeze requirements 446a5c8
Contributor

domoritz commented Aug 1, 2013

I had to fix a few pep8 issues to avoid failing tests. Also, there is a failing test in master which also fails here. Talked with @tobes about this.

Contributor

domoritz commented Aug 5, 2013

@vitorbaptista I decided to make the docs a new issue. See #1155.

Owner

amercader commented Aug 5, 2013

Some doubts around this (would be good to document them as well as part of #1155):

  • Say a dev adds a new feature that introduces a new requirement. They will develop using version external-library==1.2.3. What should they add to requirements.in? The pinned version ==1.2.3 or as a minimum version >=1.2.3?
  • I fear that small changes in requirements may have unexpected subtle consequences that could be missed on the tests. For instance, in this PR Jinja2 goes from 2.6 to 2.7, and on on the old pip-requirements.txt file it was pinned to 2.6, maybe for a reason. Is there a way to avoid that? I see that some of the requirements in requirements.in are pinned, like psycopg and sqlalchemy.
  • One point where we always have trouble is compatibility with extensions. If we are using requirements.txt for installing in production and packaging these are effectively the only versions of the CKAN requirements that extensions can use safely, am I right?
Contributor

domoritz commented Aug 5, 2013

Good points!

Say a dev adds a new feature that introduces a new requirement. They will develop using version external-library==1.2.3. What should they add to requirements.in? The pinned version ==1.2.3 or as a minimum version >=1.2.3?

They should add >=1.2.3 as long as they know that there is no newer version that we are not compatible with and document if there is. To be sure that the version is compatible, you could use >=1.2.3<1.3. I know in ruby, you can say you want a particular version and the latest patch level.

I fear that small changes in requirements may have unexpected subtle consequences that could be missed on the tests. For instance, in this PR Jinja2 goes from 2.6 to 2.7, and on on the old pip-requirements.txt file it was pinned to 2.6, maybe for a reason. Is there a way to avoid that? I see that some of the requirements in requirements.in are pinned, like psycopg and sqlalchemy.

I'm not sure about Jinja2 2.6 vs 2.7 but as long as it is compatible, I think we should use the latest version. We upgraded this particular requirement because it is not pinned in the minimum requirements.

Some requirements had to be pinned because we are not compatible with the latest version. I guess in some cases it was just lazyness not to try all possible versions and see which work. I guess, it's just an inherently difficult situation.

sqlalchemy in particular is pinned because the latest version does not work.

One point where we always have trouble is compatibility with extensions. If we are using requirements.txt for installing in production and packaging these are effectively the only versions of the CKAN requirements that extensions can use safely, am I right?

Yes.

Owner

amercader commented Aug 5, 2013

OK, @domoritz thanks for the answers. Again, this should be documented in #1155, and as it affects extensions development probably worth considering for #943 as well.

Contributor

domoritz commented Aug 5, 2013

@amercader Would you like to merge this before the docs are ready? Imho, this pr should go into 2.1 as the docs aren't necessarily tied to a certain version.

Owner

amercader commented Aug 5, 2013

Yes, I wasn't meaning to wait for the docs, I'll do a bit more testing before merging.

@domoritz domoritz added a commit that referenced this pull request Aug 5, 2013

@domoritz @amercader domoritz + amercader [#1149] Move minimum requirements into requirements.in and generate a
requirements.txt file from it using pip freeze

Conflicts:

	dev-requirements.txt
b5a2411
Owner

amercader commented Aug 6, 2013

Just as I feared, the changes in CKAN requirements cause problems and exceptions, some examples:

  • Babel 0.9.6 to 1.3: The language selector does not load the locale values (en, de, ca, ...) so you can not change the language
  • Jinja 2.6 to 2.7: Exception when creating a dataset:
File '/home/adria/dev/pyenvs/ckan_plain_req2/local/lib/python2.7/site-packages/jinja2/parser.py', line 157 in parse_statements
  result = self.subparse(end_tokens)
File '/home/adria/dev/pyenvs/ckan_plain_req2/local/lib/python2.7/site-packages/jinja2/parser.py', line 875 in subparse
  rv = self.parse_statement()
File '/home/adria/dev/pyenvs/ckan_plain_req2/local/lib/python2.7/site-packages/jinja2/parser.py', line 129 in parse_statement
  return ext(self)
File '/home/adria/dev/pyenvs/ckan_plain_req2/src/ckan/ckan/lib/jinja_extensions.py', line 65 in parse
  args = getattr(node.nodes[0], 'args', None)
AttributeError: 'list' object has no attribute 'nodes'

Both were not raised by the tests, but were discovered with just very simple testing. There are other updated versions that could potentially break things and be far more hard to find or debug (pyutilib.component.core, zope.interface, ...).

I don't feel comfortable merging this as is. Either we pin all the CKAN requirements to the versions we know work, we review exhaustively all the version changes or we think of a different approach.

Contributor

domoritz commented Aug 6, 2013

I think we have to upgrade dependencies to later versions at some point so I think the second solution, "we review exhaustively all the version changes", is the best approach. Maybe we can set up a test server somewhere that uses the requirements from this branch (I'll fix jinja2 and babel in a sec). However, that's just my opinion.

amercader was assigned Aug 6, 2013

Contributor

domoritz commented Aug 6, 2013

@amercader Okay, pinned requirements in .in file as discussed.

@amercader amercader merged commit be0faad into master Aug 6, 2013

1 check passed

default The Travis CI build passed
Details

@domoritz domoritz added a commit that referenced this pull request Aug 6, 2013

@domoritz @amercader domoritz + amercader [#1149] Move minimum requirements into requirements.in and generate a
requirements.txt file from it using pip freeze

Conflicts:

	dev-requirements.txt
fe83666

@domoritz domoritz added a commit that referenced this pull request Aug 6, 2013

@domoritz @amercader domoritz + amercader [#1149] Regenerate requirements file with older version of Babel and …
…jinja2 that do not cause errors
32b3ded

vitorbaptista deleted the 1149-pip-requirements branch Aug 9, 2013

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