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

Patch setup #182

Merged
merged 27 commits into from Sep 14, 2015

Conversation

Projects
None yet
5 participants
@benjello
Contributor

benjello commented Aug 11, 2015

No description provided.

@benjello benjello referenced this pull request Aug 11, 2015

Closed

add travis CI #135

@benjello

This comment has been minimized.

Show comment
Hide comment
@benjello

benjello Aug 11, 2015

Contributor

Response to questions raised in #135

  • Makefile: very useful for a linux box, useful for travis, do not harm others ;-)
  • Will try to fix the travis python-tables issue (@cbenz any hint ? ;-) )
  • I can put the examples where you find it more suitable, just let me know
Contributor

benjello commented Aug 11, 2015

Response to questions raised in #135

  • Makefile: very useful for a linux box, useful for travis, do not harm others ;-)
  • Will try to fix the travis python-tables issue (@cbenz any hint ? ;-) )
  • I can put the examples where you find it more suitable, just let me know
@cbenz

This comment has been minimized.

Show comment
Hide comment
@cbenz

cbenz Aug 11, 2015

Contributor
  • Makefiles are indeed non-pythonic. We are still looking for a better alternative. @gdementen We too are open for discussion ;-)
  • According to travis doc they use Ubuntu 12.04 LTS Server Edition 64 bit (codename : Precise). python-tables for Precise page shows that travis uses version 2.3.1-2ubuntu3

I see that https://github.com/benjello/liam2/blame/patch_setup/liam2/tests/test_liam2.py#L21 swallows the real error. BTW I do not have the same (I have invalid keyword(s): 'output')

@benjello I'm first going to send you another pull-request which displays the error better.

Contributor

cbenz commented Aug 11, 2015

  • Makefiles are indeed non-pythonic. We are still looking for a better alternative. @gdementen We too are open for discussion ;-)
  • According to travis doc they use Ubuntu 12.04 LTS Server Edition 64 bit (codename : Precise). python-tables for Precise page shows that travis uses version 2.3.1-2ubuntu3

I see that https://github.com/benjello/liam2/blame/patch_setup/liam2/tests/test_liam2.py#L21 swallows the real error. BTW I do not have the same (I have invalid keyword(s): 'output')

@benjello I'm first going to send you another pull-request which displays the error better.

@benjello

This comment has been minimized.

Show comment
Hide comment
@benjello

benjello Aug 11, 2015

Contributor

@cbenz: the output error is a true liam2 error ;-)

Contributor

benjello commented Aug 11, 2015

@cbenz: the output error is a true liam2 error ;-)

@@ -0,0 +1,19 @@
agegroup,gender,,

This comment has been minimized.

@gdementen

gdementen Aug 11, 2015

Member

this file shouldn't be committed

@gdementen

gdementen Aug 11, 2015

Member

this file shouldn't be committed

This comment has been minimized.

@benjello

benjello Aug 11, 2015

Contributor

Dealt with by upadating .gitignore

@benjello

benjello Aug 11, 2015

Contributor

Dealt with by upadating .gitignore

This comment has been minimized.

@gdementen

gdementen Aug 25, 2015

Member

Good to avoid that error in the future, but this particular file should still be removed from the PR.

@gdementen

gdementen Aug 25, 2015

Member

Good to avoid that error in the future, but this particular file should still be removed from the PR.

@@ -0,0 +1,19 @@
agegroup,gender,,

This comment has been minimized.

@gdementen

gdementen Aug 11, 2015

Member

idem

@gdementen

gdementen Aug 11, 2015

Member

idem

This comment has been minimized.

@benjello

benjello Aug 11, 2015

Contributor

idem

@benjello

benjello Aug 11, 2015

Contributor

idem

This comment has been minimized.

@gdementen

gdementen Aug 25, 2015

Member

idem :)

@gdementen

gdementen Aug 25, 2015

Member

idem :)

Show outdated Hide outdated liam2/tests/test_liam2.py Outdated
@cbenz

This comment has been minimized.

Show comment
Hide comment
@cbenz

cbenz Aug 11, 2015

Contributor

@benjello I think I did what I could to unlock the travis issues. Feel free to contact me again if needed :)

Contributor

cbenz commented Aug 11, 2015

@benjello I think I did what I could to unlock the travis issues. Feel free to contact me again if needed :)

@benjello

This comment has been minimized.

Show comment
Hide comment
@benjello

benjello Aug 11, 2015

Contributor

Many thanks @cbenz for solving the python-tables problem
@gdementen : some of the failing tests are due to matplotlib plots needing Qt via sip, how do you want to handle that ?

Contributor

benjello commented Aug 11, 2015

Many thanks @cbenz for solving the python-tables problem
@gdementen : some of the failing tests are due to matplotlib plots needing Qt via sip, how do you want to handle that ?

@gdementen

This comment has been minimized.

Show comment
Hide comment
@gdementen

gdementen Aug 12, 2015

Member

by installing PyQt?

On Wed, Aug 12, 2015 at 12:20 AM, Mahdi Ben Jelloul <
notifications@github.com> wrote:

Many thanks @cbenz https://github.com/cbenz for solving the
python-tables problem
@gdementen https://github.com/gdementen : some of the failing tests are
due to matplotlib plots needing Qt via sip, how do you want to handle that ?


Reply to this email directly or view it on GitHub
#182 (comment).

Member

gdementen commented Aug 12, 2015

by installing PyQt?

On Wed, Aug 12, 2015 at 12:20 AM, Mahdi Ben Jelloul <
notifications@github.com> wrote:

Many thanks @cbenz https://github.com/cbenz for solving the
python-tables problem
@gdementen https://github.com/gdementen : some of the failing tests are
due to matplotlib plots needing Qt via sip, how do you want to handle that ?


Reply to this email directly or view it on GitHub
#182 (comment).

@benjello

This comment has been minimized.

Show comment
Hide comment
@benjello

benjello Aug 12, 2015

Contributor

@gdementen you can check it here
I think we can use a workaround but I will not be able to look at it this week.
To summarize, the trvais config is ok, now we have to dela with pure liam2 issues

Contributor

benjello commented Aug 12, 2015

@gdementen you can check it here
I think we can use a workaround but I will not be able to look at it this week.
To summarize, the trvais config is ok, now we have to dela with pure liam2 issues

@landscape-bot

This comment has been minimized.

Show comment
Hide comment
@landscape-bot

landscape-bot Aug 13, 2015

Code Health
Code quality remained the same when pulling 6699cda on benjello:patch_setup into c8b3534 on liam2:master.

landscape-bot commented Aug 13, 2015

Code Health
Code quality remained the same when pulling 6699cda on benjello:patch_setup into c8b3534 on liam2:master.

Show outdated Hide outdated liam2/tests/test_liam2.py Outdated
Show outdated Hide outdated liam2/tests/test_liam2.py Outdated
@gdementen

This comment has been minimized.

Show comment
Hide comment
@gdementen

gdementen Aug 25, 2015

Member

Back from holidays (but away next week)... A few more comments...

  • the (deprecated) assignment outside functions where left on purpose to test the warning.

    Ideally, an explicit "test" (ie one assignment outside a function) should be added for that. The one you converted (agegroup) is better left like you did because it added more clutter than necessary because it was also used to test (imports/overwritten function), which added more clutter/old syntax than necessary.

  • As far as I can tell, the remaining Travis-CI issues are not "LIAM2 issues", but issues with this PR:

    • demo03.yml and demo04.yml (ImportError: No module named PyQt4)

      should add pyqt to the list of packages to install

    • import.yml (SyntaxError: invalid keyword(s): 'output')

      this is an "import file", not a model file, so it should be run with "liam2 import import.yml", and not "liam2 run import.yml" (Simulation.from_yaml will not work on an "import" file)

    • simulation.yml (TypeError: 'Variable' object is not callable)
      this is your change for agegroup to compute_agegroup which is not complete: to make it work, you need to also rename agegroup in the simulation->processes lists (in both simulation.yml and imported1.yml) and the call to it in test_hybrid. But then test_hybrid does not make much sense anymore (it would not be an hybrid variable/function anymore), so we need to introduce another hybrid variable instead.

Member

gdementen commented Aug 25, 2015

Back from holidays (but away next week)... A few more comments...

  • the (deprecated) assignment outside functions where left on purpose to test the warning.

    Ideally, an explicit "test" (ie one assignment outside a function) should be added for that. The one you converted (agegroup) is better left like you did because it added more clutter than necessary because it was also used to test (imports/overwritten function), which added more clutter/old syntax than necessary.

  • As far as I can tell, the remaining Travis-CI issues are not "LIAM2 issues", but issues with this PR:

    • demo03.yml and demo04.yml (ImportError: No module named PyQt4)

      should add pyqt to the list of packages to install

    • import.yml (SyntaxError: invalid keyword(s): 'output')

      this is an "import file", not a model file, so it should be run with "liam2 import import.yml", and not "liam2 run import.yml" (Simulation.from_yaml will not work on an "import" file)

    • simulation.yml (TypeError: 'Variable' object is not callable)
      this is your change for agegroup to compute_agegroup which is not complete: to make it work, you need to also rename agegroup in the simulation->processes lists (in both simulation.yml and imported1.yml) and the call to it in test_hybrid. But then test_hybrid does not make much sense anymore (it would not be an hybrid variable/function anymore), so we need to introduce another hybrid variable instead.

@benjello

This comment has been minimized.

Show comment
Hide comment
@benjello

benjello Aug 27, 2015

Contributor

Thanks @AlexisEidelman and @gdementen for your inputs. I am working on this PR, intermittently this week but more constantly next week.

Contributor

benjello commented Aug 27, 2015

Thanks @AlexisEidelman and @gdementen for your inputs. I am working on this PR, intermittently this week but more constantly next week.

@benjello

This comment has been minimized.

Show comment
Hide comment
@benjello

benjello Sep 8, 2015

Contributor

@gdementen : after many tries by others more competent than myself, we didn't manage to have bcolz correctly installed to run the tests correctly on travis.
Should we not run the test using bcolz on travis (if there are some left, i didn't investigate) ?
Can we find a substitute for bcolz who runs easily on travis ?

Contributor

benjello commented Sep 8, 2015

@gdementen : after many tries by others more competent than myself, we didn't manage to have bcolz correctly installed to run the tests correctly on travis.
Should we not run the test using bcolz on travis (if there are some left, i didn't investigate) ?
Can we find a substitute for bcolz who runs easily on travis ?

@gdementen

This comment has been minimized.

Show comment
Hide comment
@gdementen

gdementen Sep 10, 2015

Member

@benjello There is no substitute for bcolz that I know of. The only test which currently uses bcolz is
tests/functional/import.yml. I would be fine skipping it for now... except that most other tests (except generate.yml and the examples/) rely on it to create the input database.

There are a few options to go from here:

  1. use conda to install packages. This is the best solution IMO and I will eventually do it one day anyway (but not in the near future because I am a bit swamped right now). This would make builds faster too.

  2. you could modify the test input dataset to include the work & age columns in the .csv file and move the "interpolation" test to a separate test which could be safely skipped for now. FYI, interpolation is triggered when you don't import all fields of an entity from the same file (ie when you have a "files" keyword in your import file).

  3. only use bcolz if present and put a warning otherwise. After all, it is only used to save some memory. It used to blow up my memory when importing large databases with many periods, but the demo dataset will certainly not. This should be pretty easy to do, simply do not execute lines
    https://github.com/liam2/liam2/blob/master/liam2/importer.py#L415-419 if bcolz is not present, and it "should work".

  4. Actually, now that I am looking at the code (no time to actually test), it does not look like it would improve PEAK memory use since everything seem to be already loaded at that point, so if it didn't blow up already, it's useless to compress. If that is indeed the case (would need to check the mem usage during an import), bcolz could be removed entirely. It's probably a relic from a time where I loaded stuff more lazily.

In conclusion, please do the solution you prefer as they are all fine by me (I suppose 3 is easiest). I created a ticket for 4 (#183) so that I don't forget about it in case you don't want to tackle that.

FWIW, even if removed now, bcolz will probably come back one day in a much more prominent way, as an alternative (replacement?) for HDF5 for the on-disk format (that's why solution 1 is actually my preferred).

Member

gdementen commented Sep 10, 2015

@benjello There is no substitute for bcolz that I know of. The only test which currently uses bcolz is
tests/functional/import.yml. I would be fine skipping it for now... except that most other tests (except generate.yml and the examples/) rely on it to create the input database.

There are a few options to go from here:

  1. use conda to install packages. This is the best solution IMO and I will eventually do it one day anyway (but not in the near future because I am a bit swamped right now). This would make builds faster too.

  2. you could modify the test input dataset to include the work & age columns in the .csv file and move the "interpolation" test to a separate test which could be safely skipped for now. FYI, interpolation is triggered when you don't import all fields of an entity from the same file (ie when you have a "files" keyword in your import file).

  3. only use bcolz if present and put a warning otherwise. After all, it is only used to save some memory. It used to blow up my memory when importing large databases with many periods, but the demo dataset will certainly not. This should be pretty easy to do, simply do not execute lines
    https://github.com/liam2/liam2/blob/master/liam2/importer.py#L415-419 if bcolz is not present, and it "should work".

  4. Actually, now that I am looking at the code (no time to actually test), it does not look like it would improve PEAK memory use since everything seem to be already loaded at that point, so if it didn't blow up already, it's useless to compress. If that is indeed the case (would need to check the mem usage during an import), bcolz could be removed entirely. It's probably a relic from a time where I loaded stuff more lazily.

In conclusion, please do the solution you prefer as they are all fine by me (I suppose 3 is easiest). I created a ticket for 4 (#183) so that I don't forget about it in case you don't want to tackle that.

FWIW, even if removed now, bcolz will probably come back one day in a much more prominent way, as an alternative (replacement?) for HDF5 for the on-disk format (that's why solution 1 is actually my preferred).

@benjello

This comment has been minimized.

Show comment
Hide comment
@benjello

benjello Sep 11, 2015

Contributor

@gdementen :

  • bcolz is now a real option and do not block tests
  • as you can check there are tests that cannot be run on travis because lack of sip/pyqt4. I do not know how ta address that point.
  • other problems arise from hdf5 file writing
  • there is still a test failing on my machine

In order to have this PR proceeded, may I ask you to:

  • leave the first two issues unsolved for now ?
  • have a look at the lasting failing test ?

As usual, I can do any work fitting in my limited range of skills you would assign me (no conda).

Contributor

benjello commented Sep 11, 2015

@gdementen :

  • bcolz is now a real option and do not block tests
  • as you can check there are tests that cannot be run on travis because lack of sip/pyqt4. I do not know how ta address that point.
  • other problems arise from hdf5 file writing
  • there is still a test failing on my machine

In order to have this PR proceeded, may I ask you to:

  • leave the first two issues unsolved for now ?
  • have a look at the lasting failing test ?

As usual, I can do any work fitting in my limited range of skills you would assign me (no conda).

@gdementen

This comment has been minimized.

Show comment
Hide comment
@gdementen

gdementen Sep 11, 2015

Member

About bcolz: nice, thanks!

  • hdf5 writing issues: no idea what it's about. I can only guess it's
    because of a bad version of hdf5 lib or incompatiblity between the version
    of pytables & hdf5 lib.
  • KeyError: <type 'long'>, this is probably a linux-specific issue. I
    suppose it should be easily fixable by adding it to the list of types.
  • PyQt issues. If PyQt is not available, I guess the only option for
    now is to somehow disable those tests on Travis.

Please remove the functional test .h5 datasets from git. H5 files
shouldn't be on the repo except the one in examples. The reasoning is
that I want the experience to be as easy as possible for bundle users,
but those running from source are usually more computer-savy and could
generate the h5 themselves. Plus the example dataset is smaller than
the functional test one.

On Fri, Sep 11, 2015 at 5:16 PM, Mahdi Ben Jelloul <notifications@github.com

wrote:

@gdementen https://github.com/gdementen :

  • bcolz is now a real option and do not block tests
  • as you can check there are tests that cannot be run on travis
    because lack of sip/pyqt4. I do not know how ta address that point.
  • other problems arise from hdf5 file writing
  • there is still a test failing on my machine

In order to have this PR proceeded, may I ask you to:

  • leave the first two issues unsolved for now ?
  • have a look at the lasting failing test ?

As usual, I can do any work fitting in my limited range of skills you
would assign me (no conda).


Reply to this email directly or view it on GitHub
#182 (comment).

Member

gdementen commented Sep 11, 2015

About bcolz: nice, thanks!

  • hdf5 writing issues: no idea what it's about. I can only guess it's
    because of a bad version of hdf5 lib or incompatiblity between the version
    of pytables & hdf5 lib.
  • KeyError: <type 'long'>, this is probably a linux-specific issue. I
    suppose it should be easily fixable by adding it to the list of types.
  • PyQt issues. If PyQt is not available, I guess the only option for
    now is to somehow disable those tests on Travis.

Please remove the functional test .h5 datasets from git. H5 files
shouldn't be on the repo except the one in examples. The reasoning is
that I want the experience to be as easy as possible for bundle users,
but those running from source are usually more computer-savy and could
generate the h5 themselves. Plus the example dataset is smaller than
the functional test one.

On Fri, Sep 11, 2015 at 5:16 PM, Mahdi Ben Jelloul <notifications@github.com

wrote:

@gdementen https://github.com/gdementen :

  • bcolz is now a real option and do not block tests
  • as you can check there are tests that cannot be run on travis
    because lack of sip/pyqt4. I do not know how ta address that point.
  • other problems arise from hdf5 file writing
  • there is still a test failing on my machine

In order to have this PR proceeded, may I ask you to:

  • leave the first two issues unsolved for now ?
  • have a look at the lasting failing test ?

As usual, I can do any work fitting in my limited range of skills you
would assign me (no conda).


Reply to this email directly or view it on GitHub
#182 (comment).

@benjello

This comment has been minimized.

Show comment
Hide comment
@benjello

benjello Sep 13, 2015

Contributor

After excluding some problematic tests, travis tests pass.
More tests are available to test locally.

Contributor

benjello commented Sep 13, 2015

After excluding some problematic tests, travis tests pass.
More tests are available to test locally.

gdementen added a commit that referenced this pull request Sep 14, 2015

Merge pull request #182 from benjello/patch_setup
Moved tests, enabled TravisCI and fixed tests on Linux

@gdementen gdementen merged commit 7e47228 into liam2:master Sep 14, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gdementen

This comment has been minimized.

Show comment
Hide comment
@gdementen

gdementen Sep 14, 2015

Member

Ok, looks good enough => merging. I'll fix the minor nitpicks I still have later.
@benjello, @cbenz , @AlexisEidelman Thanks a lot for your work on this.

Member

gdementen commented Sep 14, 2015

Ok, looks good enough => merging. I'll fix the minor nitpicks I still have later.
@benjello, @cbenz , @AlexisEidelman Thanks a lot for your work on this.

@benjello benjello deleted the benjello:patch_setup branch Sep 16, 2015

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