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

Install solvers to a relevant site-package by default #517

Merged
merged 9 commits into from Sep 3, 2018

Conversation

Projects
None yet
2 participants
@randomir
Copy link
Collaborator

randomir commented Aug 12, 2018

Removes need for PYTHONPATH patching.

Bulk of work in 7d496ce, the remaining commits update docs and tests.

Addresses #495.

randomir added some commits Aug 7, 2018

Add debian-like solver install location heuristic
As discussed in #495:

- if user has virtual environment activated, simply install solvers in
  virtualenv's site packages
- otherwise (debian's pip logic):
  - if user is root, install to system site/dist packages
  - else if user has user site enabled, install solvers user-local
  - otherwise, try installing to system site packages and probably
    fail.
@randomir

This comment has been minimized.

Copy link
Collaborator Author

randomir commented Aug 13, 2018

Heads up -- all installs pass on all platforms, and all unittests succeed (check here).

The only thing currently failing are additional examples run in "all solvers" case. PySMT import fails there for some reason. Working on it (but it's so painfully slow to debug this on Travis - in the best case you need to wait for 30m+ to compile/install all solvers, just to run the actual test after that. Sometimes tests segfault, and sometimes archive download timeouts).

@randomir randomir force-pushed the randomir:env-local-install-remove-path-extension branch 7 times, most recently from e09163e to 460844d Aug 13, 2018

@randomir randomir force-pushed the randomir:env-local-install-remove-path-extension branch from 460844d to 7f7b64a Aug 14, 2018

@randomir

This comment has been minimized.

Copy link
Collaborator Author

randomir commented Aug 14, 2018

All righty, all good now. The problem was with PYTHONPATH not including ., and at the same time example tests relying on pysmt being in path (cwd).

Since we don't need to extend PYTHONPATH for solvers to work anymore, PYTHONPATH was unset. On some systems it's enough to have PYTHONPATH='' for python to include cwd in sys.path, but I added an explicit ..

Btw, tests are now modified to (by default) install all solvers according to the default bindings dir heuristic in the installer. I.e. we're also testing that works.

If you want to run tests with custom/explicit bindings dir, set PYSMT_CUSTOM_BINDINGS_PATH="TRUE" in .travis.yml.

@marcogario

This comment has been minimized.

Copy link
Contributor

marcogario commented Aug 15, 2018

Great ! Thanks for this. It will take me a few days to review this properly, but on a quick look, it looks good.


for ex in examples/*.py; do
echo $ex
python $ex

This comment has been minimized.

@marcogario

marcogario Aug 19, 2018

Contributor

This does not have the exit anymore. If the example fails, will the script terminate with an error code? If this is not the case, Travis will not know that the test failed.

This comment has been minimized.

@randomir

randomir Aug 19, 2018

Author Collaborator

Doesn't Travis use something like set -x, so the script/test terminates on the first failed command?

This loop should be functionally identical to the one you had -- where you had exit, but from a separate subshell. This: (python $ex; exit $?) is identical to python $ex (in terms of exit status).

This comment has been minimized.

@randomir

randomir Aug 19, 2018

Author Collaborator

Just to clarify, my motivation for refactoring this loop was the use of for in ls results antipattern. I replaced subshell invocations as well because I don't see the need for them here. Correct me if I'm wrong.


# since we're relying on relative `pysmt` import in examples,
# ensure `.` is added to `sys.path`
export PYTHONPATH=${PYTHONPATH:-.}

This comment has been minimized.

@marcogario

marcogario Aug 19, 2018

Contributor

Why wasn't this a problem before?

Would it make sense to pip install the library locally?

This comment has been minimized.

@randomir

randomir Aug 19, 2018

Author Collaborator

This wasn't a problem before because before PYTHONPATH was always explicitly set.

Absolutely, local pip install . is an option. I went with this solution because it required minimal changes (to stay as close to the original solution).

This comment has been minimized.

@marcogario

marcogario Aug 20, 2018

Contributor

pip install might be the cleaner version, but let's keep that for another time.

@@ -307,3 +308,70 @@ def find_python_config(self):
if command is not None:
break
return command


def package_install_site(name='', user=False, plat_specific=False):

This comment has been minimized.

@marcogario

marcogario Aug 19, 2018

Contributor

plat_specific -> platform_specific

This comment has been minimized.

@randomir

randomir Aug 19, 2018

Author Collaborator

plat_specific is standard name in distutils.

@marcogario

This comment has been minimized.

Copy link
Contributor

marcogario commented Aug 19, 2018

Looks good. Only some minor changes, I can do those.

Regarding the CI, we need to fix the caching mechanism.
Travis has an option to cache some of the build process. We do so in our travis.yml ( https://github.com/pysmt/pysmt/blob/master/.travis.yml#L246 ) for ${HOME}/python_bindings. The idea is to avoid re-compiling the solvers every time (e.g., CVC4 requires 30 minutes to build).

What directory do we need to cache now? I guess ~/.local/bin on linux ? What about macOS?

For the same reason, I re-triggered the CI on travis after deleting the cache.

@randomir

This comment has been minimized.

Copy link
Collaborator Author

randomir commented Aug 19, 2018

Since the current Travis build uses virtual env, python bindings for solvers are installed in site-packages inside that env. So maybe you can cache the complete virtual env for the project?

The path to the virtual env root is (for python 3.6 build): /home/travis/virtualenv/python3.6.3/, and path to site-packages is: /home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages. More general, the path will be in VIRTUAL_ENV environment variable.

If you want me to patch something, just let me know. From the comments above, I'm not exactly sure what would you like to be changed... Goes without saying, please feel free to modify/amend my PR to your liking.

@randomir

This comment has been minimized.

Copy link
Collaborator Author

randomir commented Aug 28, 2018

@marcogario, any idea why that z3 test keeps segfaulting? I've restarted it several times. Maybe something is cached that's not working?

@marcogario

This comment has been minimized.

Copy link
Contributor

marcogario commented Sep 3, 2018

This is a known issue that we have been trying to isolate for a while. Nothing to do with yhis particular pr. It is difficult to debug because I managed to reproduce it only once on my machine and otherwise it is non deterministic. One idea of the cause is the an incorrect use of the refcount mechanism in our z3 converter. I was reviewing it a few weeks ago, but did not find anything suspicious. We might try to upgrade z3, if there is a more recent version available.

@marcogario marcogario merged commit 20c377d into pysmt:master Sep 3, 2018

3 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@arcondello arcondello referenced this pull request Oct 1, 2018

Open

PySMT version conflict #61

yoni206 added a commit to yoni206/pysmt that referenced this pull request Oct 3, 2018

Install solvers to a relevant site-package by default (pysmt#517)
- if user has virtual environment activated, simply install solvers in
  virtualenv's site packages
- otherwise (debian's pip logic):
  - if user is root, install to system site/dist packages
  - else if user has user site enabled, install solvers user-local
  - otherwise, try installing to system site packages and probably
    fail.

* Update help/docs/readme to reflect new bindings dir
* New bindings dir default in solver installers
* Add pip-like default package location heuristic helper
* Add debian-like solver install location heuristic
* Installer creates bindings/install dir recursively
* Use default bindings dir in Travis/Appveyor tests
* Fix example runner in tests (no ls, no subshell)
* Patch python path for examples in tests
* CI: Caching venv

@marcogario marcogario added this to the 0.7.6 milestone Nov 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.