Opened 5 years ago

Closed 4 years ago

#20736 closed enhancement (fixed)

Upgrade patchbot to 2.6.1 as a system package

Reported by: chapoton Owned by:
Priority: major Milestone: sage-7.6
Component: packages: optional Keywords: patchbot
Cc: embray, jdemeyer Merged in:
Authors: Frédéric Chapoton Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: c9c4101 (Commits) Commit: c9c4101b961c454b2acac23508a74ccfbcffb7c4
Dependencies: #22473 Stopgaps:

Description (last modified by jdemeyer)

The patchbot is currently installing in a very not kosher way, as explained in

https://github.com/robertwb/sage-patchbot/issues/79

This ticket will change completely the installation mode. The patchbot will become a python package, to be installed using pip in the system python as explained below.

This ticket also updates the patchbot, in particular to cope with the new https trac.

New tarball for patchbot: http://www-irma.u-strasbg.fr/~chapoton/sage-patchbot-2.6.1.tar.gz

installation (two equivalent methods at your disposal):

  1. Download the tarball and then
    pip install --user sage-patchbot-2.6.1.tar.gz
    
  1. (recommended)
    pip install --user git+https://github.com/sagemath/sage-patchbot.git@2.6.1
    

There are now two possible ways to launch the patchbot: 1.

python -m sage_patchbot.patchbot --sage-root=XXXX

or

  1. Checkout the branch here and run
    make && ./sage --patchbot
    

Change History (163)

comment:1 Changed 5 years ago by chapoton

  • Description modified (diff)

comment:2 Changed 5 years ago by chapoton

  • Branch set to public/20736
  • Commit set to a827087d9b4335f351605f2d2a119b53d0c3bd25

I tried and failed so far. Help is required, please.


New commits:

a827087trac 20736 first try, failing

comment:3 Changed 5 years ago by embray

Not related to installation problem, but in the dependencies file you should put

$(PYTHON) | setuptools

That is, if setuptools is reinstalled it's not necessary to reinstall a Python package that happens to use it. But it is a prerequisite to installing.

comment:4 Changed 5 years ago by git

  • Commit changed from a827087d9b4335f351605f2d2a119b53d0c3bd25 to c9907efed9c7f02a928af40a2514a28f2ed7e356

Branch pushed to git repo; I updated commit sha1. New commits:

c9907eftrac 20736 change in dependencies

comment:5 Changed 5 years ago by embray

BTW, when building a source package of a Python project, use ./setup.py sdist to make the tarball (also make sure the tarball contains all the files it should contain, and none that it shouldn't contain--you might have to edit a MANIFEST.in file to adjust it a la https://docs.python.org/2/distutils/sourcedist.html#specifying-the-files-to-distribute)

comment:6 Changed 5 years ago by chapoton

  • Description modified (diff)

comment:7 Changed 5 years ago by git

  • Commit changed from c9907efed9c7f02a928af40a2514a28f2ed7e356 to f3839fd6c0fb2562fb7731e0a62f73a2d3b89124

Branch pushed to git repo; I updated commit sha1. New commits:

f3839fdanother try, failing the same way

comment:8 Changed 5 years ago by chapoton

I am making some slow progress.

Last edited 5 years ago by chapoton (previous) (diff)

comment:9 Changed 5 years ago by chapoton

  • Description modified (diff)

comment:10 Changed 5 years ago by jdemeyer

There is a fundamental problem here, mentioned also at https://github.com/robertwb/sage-patchbot/issues/79

We cannot use the Sage Python for the patchbot, since the patchbot might reinstall Python. So I don't know what the best solution is, but installing it "the usual way" is not the right thing to do here.

comment:11 Changed 5 years ago by git

  • Commit changed from f3839fd6c0fb2562fb7731e0a62f73a2d3b89124 to 160332c495872a01047d109a8584aca955d98252

Branch pushed to git repo; I updated commit sha1. New commits:

160332cfixing the spkg-install script

comment:12 follow-up: Changed 5 years ago by embray

So it's a build dep for Sage and should be installed "the usual way" using the system Python.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 5 years ago by jdemeyer

Replying to embray:

So it's a build dep for Sage and should be installed "the usual way" using the system Python.

I don't get why you say a "build dep", but I agree with the second part.

comment:14 follow-up: Changed 5 years ago by chapoton

I changed the name of the package back to sage-patchbot. But I am little bit stuck here.

It kind of worked with the package name "patchbot", but it was installed in SAGE_LOCAL.

comment:15 in reply to: ↑ 13 Changed 5 years ago by embray

Replying to jdemeyer:

Replying to embray:

So it's a build dep for Sage and should be installed "the usual way" using the system Python.

I don't get why you say a "build dep", but I agree with the second part.

"build dep" is probably the wrong description--but I just meant a developer tool for development of Sage, outside the sage distribution.

comment:16 in reply to: ↑ 14 Changed 5 years ago by embray

Replying to chapoton:

I changed the name of the package back to sage-patchbot. But I am little bit stuck here.

It kind of worked with the package name "patchbot", but it was installed in SAGE_LOCAL.

To be clear, this is a case where the term "package" is often overloaded in Python:

  • Here the package, as in the name of what you import in Python is patchbot
  • The name in the setup.py is different from the package name, and as sometimes referred to as the "distribution name" or the "PyPI name" to reduce confusion. Often the package name and the distribution name are the same, but they don't have to be. Either way it shouldn't be relevant here.

I think the main takeaway here is that the patchbot should not be a Sage spkg at all. It's not a part of sage, the distribution. It's a tool for testing sage the distribution and as such should be entirely external to it.

So to install the patchbot on say a Linux system one would use sudo pip install <path/to/tarball> or even just sudo pip install sage-patchbot if the latest version is uploaded to PyPI.

Alternatively this can be done within a virtualenv if we prefer to avoid the system install.

comment:17 Changed 5 years ago by chapoton

  • Commit changed from 160332c495872a01047d109a8584aca955d98252 to 228842b1aad446925645dcadb72f9c768358fe0d
  • Description modified (diff)
  • Summary changed from make patchbot a correct python package to upgrade patchbot to 2.6.0 (compatible with https trac)

New commits:

eed116fMerge branch 'public/20736' into 7.3.b4
228842bupdate of patchbot to 2.6.0

comment:18 Changed 5 years ago by chapoton

This is becoming urgent, to cope with https trac.

I tried to go back to the previous way but this is not working. It complains about

sage -patchbot --ticket=20862
Traceback (most recent call last):
  File "sage/local/bin/patchbot/patchbot.py", line 63, in <module>
    from .trac import scrape, pull_from_trac, TracServer, Config
ValueError: Attempted relative import in non-package

I need help. Should I just remove all relative imports newly introduced ?? should I keep the strange source dir of patchbot inside the local/bin directory ??

comment:19 Changed 5 years ago by chapoton

  • Description modified (diff)

comment:20 Changed 5 years ago by chapoton

  • Description modified (diff)

I am trying a new way:

  • have a tarball made with "python ./setup.py sdist"
  • install it using "pip install sage-patchbot-tarball"
  • run it as usual by "sage -patchbot" (with sage on the current branch)
Last edited 5 years ago by chapoton (previous) (diff)

comment:21 Changed 5 years ago by chapoton

  • Description modified (diff)

comment:22 in reply to: ↑ description ; follow-up: Changed 5 years ago by jdemeyer

  • Commit changed from 228842b1aad446925645dcadb72f9c768358fe0d to 91fc4927d5115ffd9af20b45b9cc5d05a4b284ef

Replying to chapoton:

usage: sudo pip install sage-patchbot-2.6.0.tar.gz

VERY -1 to this!


New commits:

91fc492patchbot : trying to use pip install

comment:23 Changed 5 years ago by jdemeyer

Can you please update the patchbot to only deal with the https thing and not change the whole install procedure?

comment:24 follow-up: Changed 5 years ago by chapoton

I am doing what I can, but this is a bit too complicated for me to handle easily.

I changed all the imports in such a way that patchbot must now be used a a python package. The simpplest way (and it works) is what I suggest above.

Why do you so strongly object to pip installation in the system python ?

Now if you have better suggestions, I would appreciate help.

comment:25 in reply to: ↑ 24 ; follow-up: Changed 5 years ago by jdemeyer

Replying to chapoton:

I changed all the imports in such a way that patchbot must now be used a a python package.

I don't understand how the imports should be related to how the package is installed.

Why do you so strongly object to pip installation in the system python ?

It should be possible to install and run the patchbot without sudo access.

comment:26 in reply to: ↑ 25 ; follow-ups: Changed 5 years ago by chapoton

Replying to jdemeyer:

Replying to chapoton:

I changed all the imports in such a way that patchbot must now be used a a python package.

I don't understand how the imports should be related to how the package is installed.

Relative imports are not allowed when a python file is run as a script, it seems.

Why do you so strongly object to pip installation in the system python ?

It should be possible to install and run the patchbot without sudo access.

How is it compatible with not installing in the sage python ?

Is there any way to install somewhere inside sage a python package using the system python ? Is there any existing case ? Where should it go ? How should I do that ?

comment:27 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from upgrade patchbot to 2.6.0 (compatible with https trac) to Upgrade patchbot to 2.6.0 to system package

comment:28 in reply to: ↑ 26 Changed 5 years ago by jdemeyer

Replying to chapoton:

Relative imports are not allowed when a python file is run as a script, it seems.

That can't be true. You're probably just missing some __init__.py file.

comment:29 in reply to: ↑ 26 Changed 5 years ago by jdemeyer

Replying to chapoton:

Is there any way to install somewhere inside sage a python package using the system python ? Is there any existing case ? Where should it go ? How should I do that ?

User installs are possible with pip, I edited the ticket description.

comment:30 follow-up: Changed 5 years ago by jdemeyer

If the patchbot becomes a system package, it should be removed as Sage package. I.e. remove the whole directory build/pkgs/patchbot.

comment:31 in reply to: ↑ 30 ; follow-up: Changed 5 years ago by chapoton

Replying to jdemeyer:

If the patchbot becomes a system package, it should be removed as Sage package. I.e. remove the whole directory build/pkgs/patchbot.

yes, I agree.

And there is an __init__.py in the package.

comment:32 in reply to: ↑ 31 Changed 5 years ago by jdemeyer

  • Commit changed from 91fc4927d5115ffd9af20b45b9cc5d05a4b284ef to 897f5b220c7eb63a994853092be376d62e40fe39

Replying to chapoton:

And there is an __init__.py in the package.

Well, if you want me to have a look, I need more details. But if it works as intended outside of Sage with a user install, that's good enough.


New commits:

897f5b2trac 20736 removing patchbot as spkg

comment:33 Changed 5 years ago by embray

If installed correctly there should be no sage/local/bin/patchbot/patchbot.py

The exception you're getting should be unrelated to the new patchbot package. Instead, the old patchbot spkg should *not* be getting installed.

comment:34 in reply to: ↑ 22 Changed 5 years ago by embray

Replying to jdemeyer:

Replying to chapoton:

usage: sudo pip install sage-patchbot-2.6.0.tar.gz

VERY -1 to this!


New commits:

91fc492patchbot : trying to use pip install

+1 to this. This is an appropriate way to install a Python package that is not (and will likely never be) provided by the system packager. Debian, at least, handles this well.

The alternative would be some kind of ~/.local install--need to ensure that the PYTHONPATH is correct when running the patchbot though.

As I've written before on this thread the patchbot should not be a Sage spkg.

comment:35 follow-ups: Changed 5 years ago by chapoton

  • Status changed from new to needs_review

The 2.6.0 seems to work well enough on sage4, although at some point it tested closed tickets.

On the other hand, arando is uselessly running 2.5.7.

comment:36 in reply to: ↑ 35 Changed 5 years ago by jdemeyer

Replying to chapoton:

On the other hand, arando is uselessly running 2.5.7.

I know, I have not upgraded it yet.

comment:37 in reply to: ↑ 35 Changed 5 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work

Replying to chapoton:

The 2.6.0 seems to work well enough on sage4, although at some point it tested closed tickets.

I did have to manually change PYTHONPATH for it to work in a user install.

I think you should be able to do

./sage --pip install --user patchbot

However, since Sage sets PYTHONUSERBASE (no idea why), this patchbot is installed in a directory different from what the system Python expects. To get it working, I had to set PYTHONPATH to the site-packages directory in ~/.sage/local. Because of confusion between lib and lib64, just setting PYTHONUSERBASE system-wide was not sufficient.

For this reason, I believe that ./sage --patchbot should set PYTHONPATH before running the patchbot.

comment:38 follow-up: Changed 5 years ago by embray

I don't know why you would run ./sage --patchbot at all, as opposed to just patchbot. Why is it necessary to use sage to run the thing that's supposed to be testing sage?

comment:39 Changed 5 years ago by chapoton

I propose the following "modus operandi" for patchbot installation and use

  • get the latest tarball.tar.gz from somewhere (git or my web page?)
  • pip install --user tarball.tar.gz
  • sage -patchbot --options (1) OR python -m sage_patchbot.patchbot --options (2)

Everything (pip and python) should take place in the system python. I do not see any reason to involve the inner python of sage.

I am not completely sure if this already works with this branch, but it was my vision of things. I think that I did (1) or (2) on my own patchbot client, which seems to be running well.

Now Jeroen and Eric do not seem to agree on what to do. Please consider that I have not a lot of time to spend on that.

comment:40 Changed 5 years ago by chapoton

much more annoying is the fact that the patchbot persist to test closed tickets..

comment:41 Changed 5 years ago by embray

get the latest tarball.tar.gz from somewhere (git or my web page?)

Why not put it on PyPI, and put out some proper releases?

Alternatively, pip allows download direct from a git repository, including specific tags.

pip install --user tarball.tar.gz

Whatever; people can pip install however they're comfortable with.

Everything (pip and python) should take place in the system python. I do not see any reason to involve the inner python of sage.

+1

I am not completely sure if this already works with this branch, but it was my vision of things. I think that I did (1) or (2) on my own patchbot client, which seems to be running well.

I don't know. Is there anything in the patchbot that tries to import sage? I don't think there is--as far as I can tell the patchbot can run entirely outside of the sage environment, as it should.

Now Jeroen and Eric do not seem to agree on what to do. Please consider that I have not a lot of time to spend on that.

AFAICT the only disagreement is over whether or not running sudo pip install is ever appropriate. My point is that it doesn't really matter--as long as the user is installing into a Python outside of the Sage distribution (be it a virtualenv, a --user install, or whatever) it should work fine. It doesn't really matter and the most appropriate choice depends on the context.

For that matter, there's probably nothing wrong with installing the patchbot package into Sage's Python too, again just using pip install. But in general it should not be necessary to.

comment:42 Changed 5 years ago by chapoton

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Summary changed from Upgrade patchbot to 2.6.0 to system package to Upgrade patchbot to 2.6.1 as a system package

Let us try to move forward, please. This works, as far as I can tell.

The patchbot package is installed in the system python, not as root but as user.

I would prefer not to use pypi for the moment.

Could somebody try to follow exactly the instructions and see if everything runs smoothly ?

comment:43 follow-up: Changed 5 years ago by vdelecroix

If I install the package on my system Python, then doing

$ python -m sage_patchbot.patchbot

I got

Traceback (most recent call last):
  File "/usr/lib/python2.7/runpy.py", line 162, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/nfs4/home6/vdelecro/.local/lib/python2.7/site-packages/sage_patchbot/patchbot.py", line 1471, in <module>
    main(args)
  File "/nfs4/home6/vdelecro/.local/lib/python2.7/site-packages/sage_patchbot/patchbot.py", line 1401, in main
    patchbot = Patchbot(os.path.abspath(options.sage_root),
  File "/usr/lib/python2.7/posixpath.py", line 367, in abspath
    if not isabs(path):
  File "/usr/lib/python2.7/posixpath.py", line 61, in isabs
    return s.startswith('/')
AttributeError: 'NoneType' object has no attribute 'startswith'

comment:44 follow-up: Changed 5 years ago by vdelecroix

All right, the command *must* specify the sage root as

$ python -m sage_patchbot.patchbot --sage-root=WHATEVER

(or setting SAGE_ROOT environment variable). I think there should be a more specific error message than what I got in comment:43. Note that if the package is installed in Sage python then it will work out of the box since SAGE_ROOT is defined.

I don't understand why you do not want to put it on PyPI. I can do it myself if you do not want to investigate. But it really takes less than a minute and greatly simplify the installation, version upgrade, online documentation, ...

Last edited 5 years ago by vdelecroix (previous) (diff)

comment:45 Changed 5 years ago by vdelecroix

(I launch the package version of the patchbot on librae)

comment:46 Changed 5 years ago by vdelecroix

I guess that the log should not go under SAGE_ROOT/logs/patchbot anymore now. The path should be part of the configuration file. What do you think?

comment:47 Changed 5 years ago by vdelecroix

What about switching to logging Python module?

comment:48 in reply to: ↑ 43 ; follow-ups: Changed 5 years ago by chapoton

Replying to vdelecroix:

If I install the package on my system Python, then doing

$ python -m sage_patchbot.patchbot

Thanks for the feedback.

These are not the instructions on how to run the patchbot given in the ticket description.

But this will be another possible way.

Librae is meeting a whole bunch of doctest failure with lrslib. Maybe it should be checked that the bot is not causing them.

Concerning pypi, it may be simpler once done, but I am not really ready to add yet another layer of complexity.

Concerning the log, I would prefer to postpone the problem, once the new setting is in place.

comment:49 in reply to: ↑ 44 Changed 5 years ago by embray

Replying to vdelecroix:

I don't understand why you do not want to put it on PyPI. I can do it myself if you do not want to investigate. But it really takes less than a minute and greatly simplify the installation, version upgrade, online documentation, ...

I don't think it's deeply necessary--pip can install fine from a git repository for example (including specific tags). But it's not a big deal either way.

And like I also wrote earlier it shouldn't matter where pathbot is installed either as long as it's installed correctly and running patchbot from the command-line works.

comment:50 in reply to: ↑ 48 Changed 5 years ago by embray

Replying to chapoton:

Concerning pypi, it may be simpler once done, but I am not really ready to add yet another layer of complexity.

You've already fixed up the patchbot to use distutils properly, so this is a matter of making an account on PyPI, setting up your credentials, and running ./setup.py register upload. There's no additional layer of complexity.

comment:51 in reply to: ↑ 48 Changed 5 years ago by vdelecroix

Replying to chapoton:

Replying to vdelecroix:

If I install the package on my system Python, then doing

$ python -m sage_patchbot.patchbot

Thanks for the feedback.

These are not the instructions on how to run the patchbot given in the ticket description.

But this will be another possible way.

Indeed. If the patchbot is a Python package it is much more natural to use it without the Sage environment variables set.

Librae is meeting a whole bunch of doctest failure with lrslib. Maybe it should be checked that the bot is not causing them.

Not the bot: #21012

Concerning pypi, it may be simpler once done, but I am not really ready to add yet another layer of complexity.

It is not a new layer. It is simply a way to distribute the module. Instead of having a tarball on your webpage there is a tarball on PyPI. It is just simpler because for installation you would just have to do pip install sage-patchbot --user (downloading would be automatic). And for upgrade pip install sage-patchbot --upgrade.

Concerning the log, I would prefer to postpone the problem, once the new setting is in place.

Of course this was not intended to be for this ticket. Still it is a question.

comment:52 Changed 5 years ago by vdelecroix

The patchbot librae is hanging on #19092 since more than a day

The current date is Fri Jul 22 21:25:47 CEST 2016. The end of logs/patchbot/patchbot.log

[2016-07-21 23:38:46] Getting ticket list...
[2016-07-21 23:39:22] testing found ticket #19092
[2016-07-21 23:39:22] #19092: init phase

last lines in logs/install.log

[dochtml] [tutorial ] copying extra files... done
[dochtml] [tutorial ] dumping search index in German (code: de) ... done
[dochtml] [tutorial ] dumping object inventory... done
[dochtml] [tutorial ] build succeeded.
[dochtml] Build finished.  The built documents can be found in /home/vincent/sage_patchbot/local/share/doc/sage/html/de/tutorial

After a Ctrl-C I had the following traceback (appearing only in logs/install.log)

[dochtml] Process PoolWorker-173:
[dochtml] Process PoolWorker-178:
[dochtml] Process PoolWorker-174:
[dochtml] Process PoolWorker-177:
[dochtml] Traceback (most recent call last):
[dochtml] Traceback (most recent call last):
[dochtml] Traceback (most recent call last):
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/process.py", line 258, in _bootstrap
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/process.py", line 258, in _bootstrap
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/process.py", line 258, in _bootstrap
[dochtml] Process PoolWorker-176:
[dochtml] Process PoolWorker-175:
[dochtml] Traceback (most recent call last):
[dochtml] Traceback (most recent call last):
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/process.py", line 258, in _bootstrap
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/process.py", line 258, in _bootstrap
[dochtml] Traceback (most recent call last):
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/process.py", line 258, in _bootstrap
[dochtml]     self.run()
[dochtml]     self.run()
[dochtml]     self.run()
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/process.py", line 114, in run
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/process.py", line 114, in run
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/process.py", line 114, in run
[dochtml]     self.run()
[dochtml]     self._target(*self._args, **self._kwargs)
[dochtml]     self._target(*self._args, **self._kwargs)
[dochtml]     self._target(*self._args, **self._kwargs)
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/process.py", line 114, in run
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/pool.py", line 102, in worker
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/pool.py", line 102, in worker
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/pool.py", line 102, in worker
[dochtml]     self.run()
[dochtml]     self._target(*self._args, **self._kwargs)
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/process.py", line 114, in run
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/pool.py", line 102, in worker
[dochtml]     self._target(*self._args, **self._kwargs)
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/pool.py", line 102, in worker
[dochtml]     self.run()
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/process.py", line 114, in run
[dochtml]     self._target(*self._args, **self._kwargs)
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/pool.py", line 102, in worker
[dochtml]     task = get()
[dochtml]     task = get()
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/queues.py", line 376, in get
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/queues.py", line 376, in get
[dochtml]     task = get()
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/queues.py", line 376, in get
[dochtml]     task = get()
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/queues.py", line 376, in get
[dochtml]     task = get()
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/queues.py", line 376, in get
[dochtml]     task = get()
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/queues.py", line 378, in get
[dochtml]     return recv()
[dochtml]   File "src/cysignals/signals.pyx", line 225, in cysignals.signals.python_check_interrupt (build/src/cysignals/signals.c:2429)
[dochtml]     racquire()
[dochtml]     racquire()
[dochtml]   File "src/cysignals/signals.pyx", line 225, in cysignals.signals.python_check_interrupt (build/src/cysignals/signals.c:2429)
[dochtml]   File "src/cysignals/signals.pyx", line 225, in cysignals.signals.python_check_interrupt (build/src/cysignals/signals.c:2429)
[dochtml]     racquire()
[dochtml]     racquire()
[dochtml]   File "src/cysignals/signals.pyx", line 225, in cysignals.signals.python_check_interrupt (build/src/cysignals/signals.c:2429)
[dochtml]   File "src/cysignals/signals.pyx", line 225, in cysignals.signals.python_check_interrupt (build/src/cysignals/signals.c:2429)
[dochtml]     racquire()
[dochtml]   File "src/cysignals/signals.pyx", line 225, in cysignals.signals.python_check_interrupt (build/src/cysignals/signals.c:2429)
[dochtml]   File "src/cysignals/signals.pyx", line 96, in cysignals.signals.sig_raise_exception (build/src/cysignals/signals.c:1116)
[dochtml]   File "src/cysignals/signals.pyx", line 96, in cysignals.signals.sig_raise_exception (build/src/cysignals/signals.c:1116)
[dochtml]   File "src/cysignals/signals.pyx", line 96, in cysignals.signals.sig_raise_exception (build/src/cysignals/signals.c:1116)
[dochtml]   File "src/cysignals/signals.pyx", line 96, in cysignals.signals.sig_raise_exception (build/src/cysignals/signals.c:1116)
[dochtml]   File "src/cysignals/signals.pyx", line 96, in cysignals.signals.sig_raise_exception (build/src/cysignals/signals.c:1116)
[dochtml] KeyboardInterrupt
[dochtml] KeyboardInterrupt
[dochtml]   File "src/cysignals/signals.pyx", line 96, in cysignals.signals.sig_raise_exception (build/src/cysignals/signals.c:1116)
[dochtml] KeyboardInterrupt
[dochtml] KeyboardInterrupt
[dochtml] KeyboardInterrupt
[dochtml] KeyboardInterrupt
[dochtml] Error building the documentation.
[dochtml] Traceback (most recent call last):
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/runpy.py", line 162, in _run_module_as_main
[dochtml]     "__main__", fname, loader, pkg_name)
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/runpy.py", line 72, in _run_code
[dochtml]     exec code in run_globals
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python2.7/site-packages/sage_setup/docbuild/__main__.py", line 2, in <module>
[dochtml]     main()
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 1630, in main
[dochtml]     builder()
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 298, in _wrapper
[dochtml]     x.get(99999)
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/pool.py", line 561, in get
[dochtml]     self.wait(timeout)
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/multiprocessing/pool.py", line 556, in wait
[dochtml]     self._cond.wait(timeout)
[dochtml]   File "/home/vincent/sage_patchbot/local/lib/python/threading.py", line 359, in wait
[dochtml]     _sleep(delay)
[dochtml]   File "src/cysignals/signals.pyx", line 225, in cysignals.signals.python_check_interrupt (build/src/cysignals/signals.c:2429)
[dochtml]   File "src/cysignals/signals.pyx", line 96, in cysignals.signals.sig_raise_exception (build/src/cysignals/signals.c:1116)
[dochtml] KeyboardInterrupt
make[1]: *** [doc-html] Error 1

comment:53 Changed 5 years ago by vdelecroix

Maybe due to swap (I need to check)...

comment:54 in reply to: ↑ 38 Changed 5 years ago by jdemeyer

Replying to embray:

I don't know why you would run ./sage --patchbot at all

Mainly just for convenience. It's easier to type ./sage --patchbot than python -m sage_patchbot.patchbot --sage-root=SAGE_ROOT.

Second reason: on most systems, python is installed system-wide. But pip often is not.

comment:55 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_info

Please update the ticket description with the recommended way to install and run the patchbot. Then I will test it.

comment:56 Changed 5 years ago by jdemeyer

You should also decide whether you want to support ./sage --patchbot or not. If you do, it should actually work (see comment:37). If you don't, remove the command (or better: make it print some message of which commands should actually be run).

comment:57 Changed 4 years ago by chapoton

  • Description modified (diff)

@jdemeyer

I have tried to enhance the description of the ticket.

I am not willing to avoid the dependency to pip.

comment:58 Changed 4 years ago by jdemeyer

  • Description modified (diff)

Like I said before, ./sage --patchbot does not actually work.

comment:59 Changed 4 years ago by jdemeyer

See the last line of 37

comment:60 Changed 4 years ago by vdelecroix

Indeed the command ./sage --patchbot has no clue where the patchbot is actually installed. It might be on system python, in Sage python, in a virtualenv, or something else... I think it is much safer to disable it completely with an error message (as proposed in comment:56).

Last edited 4 years ago by vdelecroix (previous) (diff)

comment:61 Changed 4 years ago by jdemeyer

I think it makes sense to assume that, if you run ./sage --patchbot, you want to run the patchbot which is installed from a Sage shell. That would just require some part of sage-env to be executed before running the patchbot.

comment:62 Changed 4 years ago by git

  • Commit changed from 897f5b220c7eb63a994853092be376d62e40fe39 to 14fb5998257fe1f7bc352d92c0de9571ae85a11d

Branch pushed to git repo; I updated commit sha1. New commits:

80a58baMerge branch 'public/20736' in 7.3.rc0
14fb599trac 20736 more information when calling sage --patchbot

comment:63 Changed 4 years ago by chapoton

It seems that we are not making progress here. Jeroen, why do you insist so strongly on installing the patchbot package inside the sage shell ? It makes not much sense to me. I think one can reasonably expect anybody wanting to run a patchbot to be able to install pip first. There is no need to call "sage -pip".

I propose the following alternative:

(1) disable completely "sage --patchbot"

(2) use "sage --patchbot" as a convenient way to launch the patchbot installed in the system python.

I vote for (2), which is the current situation with the branch. I have just added some explanation text to appear when "sage --patchbot" is called.

comment:64 Changed 4 years ago by jdemeyer

Jeroen, why do you insist so strongly on installing the patchbot package inside the sage shell ?

I am not really insisting on that. I am insisting on not requiring a system install. But I can live with a user install outside of Sage.

comment:65 Changed 4 years ago by jdemeyer

To turn the question around: why do you keep insisting on not supporting a patchbot installed within Sage?

comment:66 follow-up: Changed 4 years ago by chapoton

I do not insist on that either, I just do not know how to do it, and I am happy enough with the current state of things. I would prefer to stop spending my time on this ticket, and to get more momentum towards python 3 instead.

comment:67 follow-up: Changed 4 years ago by vdelecroix

It is supported with

$ sage -pip install my_patchbot_tarball.tar.gz
$ sage -python -m sage_patchbot.patchbot --sage-root=my_sage_root

It is just the command sage -patchbot which is half broken. So I would vote for (1) that is much cleaner and solve the issue.

EDIT: in my initial post I wrote (2) instead of (1)... sorry

Last edited 4 years ago by vdelecroix (previous) (diff)

comment:68 Changed 4 years ago by chapoton

Jeroen, do you also prefer (1) ?

comment:69 in reply to: ↑ 67 ; follow-up: Changed 4 years ago by jdemeyer

Replying to vdelecroix:

It is supported with

$ sage -pip install my_patchbot_tarball.tar.gz
$ sage -python -m sage_patchbot.patchbot --sage-root=my_sage_root

No, that does not really work since you cannot use the Sage Python to run the patchbot.

comment:70 in reply to: ↑ 69 ; follow-up: Changed 4 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

It is supported with

$ sage -pip install my_patchbot_tarball.tar.gz
$ sage -python -m sage_patchbot.patchbot --sage-root=my_sage_root

No, that does not really work since you cannot use the Sage Python to run the patchbot.

Why is that? It (seems to) work for me.

comment:71 in reply to: ↑ 66 Changed 4 years ago by jdemeyer

Replying to chapoton:

I do not insist on that either, I just do not know how to do it

Good. I will look into this then.

comment:72 Changed 4 years ago by git

  • Commit changed from 14fb5998257fe1f7bc352d92c0de9571ae85a11d to 654e5788ec2e32f06ef246f09ade24dc6ddf20bb

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

654e578Improve --patchbot command

comment:73 Changed 4 years ago by jdemeyer

First attempt to support a patchbot installed within Sage, not fully tested yet.

comment:74 in reply to: ↑ 70 Changed 4 years ago by jdemeyer

Replying to vdelecroix:

Why is that? It (seems to) work for me.

...until the patchbot tests a ticket which re-installs the Python within Sage.

comment:75 Changed 4 years ago by jdemeyer

Hang on, I have a better solution in mind to run the patchbot.

comment:76 Changed 4 years ago by git

  • Commit changed from 654e5788ec2e32f06ef246f09ade24dc6ddf20bb to 6d7990e88c9f194f28309bd0015c40ecc09080bf

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6d7990eImprove --patchbot command

comment:77 Changed 4 years ago by jdemeyer

  • Status changed from needs_info to needs_review

Currently testing...

comment:78 Changed 4 years ago by jdemeyer

This is good for me. Can anybody review my commit?

comment:79 Changed 4 years ago by chapoton

does not work for me:

0) pull the branch here and make
1) pip install --user git+https://github.com/sagemath/sage-patchbot.git@2.6.1
2) ./sage --patchbot
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ImportError: No module named sage_patchbot
Error: cannot find installation path for sage_patchbot
See https://wiki.sagemath.org/buildbot/details for instructions

but "import sage_patchbot" works in the system python

Last edited 4 years ago by chapoton (previous) (diff)

comment:80 follow-up: Changed 4 years ago by vdelecroix

Instead of a sloppy script, I would be very much happier with sage -patchbot returning an informative error message.

comment:81 in reply to: ↑ 80 Changed 4 years ago by embray

Replying to vdelecroix:

Instead of a sloppy script, I would be very much happier with sage -patchbot returning an informative error message.

+1

As I understand it the purpose of ./sage -foo is to run the foo program that's installed in the sage environment. I don't see why it should be doing anything else. At most if ./sage -patchbot is meant to do something it can raise an error if patchbot does not happen to be installed in the Sage environment. Then if one wants they can ./sage -pip install sage-patchbot and get it in their Sage environment, at which point ./sage -patchbot is the equivalent of . sage-env; patchbot, etc...

Otherwise all one needs to do to run the patchbot is to run patchbot however it happens to be installed on a (correctly configured) system.

comment:82 follow-up: Changed 4 years ago by rws

So what exactly is necessary to make this branch work, i.e. to run sage -patchbot successfully?

comment:83 Changed 4 years ago by rws

Answer: ./sage -pip install sage-patchbot or the downloaded 2.6.1 tarball as argument. This means the wiki instructions need to change which I just did, please check.

comment:84 Changed 4 years ago by chapoton

Please do not interfere with the wiki instructions until this ticket is closed. Revert your changes there, please.

comment:85 Changed 4 years ago by chapoton

Thanks. The current branch should be considered as broken. The wiki instructions were perfectly working until Jeroen made his new branch.

EDIT: a working branch is at u/chapoton/20736

Last edited 4 years ago by chapoton (previous) (diff)

comment:86 in reply to: ↑ 82 Changed 4 years ago by jdemeyer

Replying to rws:

So what exactly is necessary to make this branch work, i.e. to run sage -patchbot successfully?

That's easy: I just need to check both the Python-in-Sage and the system Python.

However, there seems to be disagreement (see @vdelecroix and @embray) about what ./sage --patchbot should actually do. There is no point in fixing the branch if I have to throw it away anyway.

comment:87 Changed 4 years ago by chapoton

Proposed solution:

  • do not allow sage -patchbot for running the bot
  • make so that sage -patchbot just prints:
    The patchbot can no longer be run in this way.
    Please see https://wiki.sagemath.org/buildbot/details for up-to-date instructions.
    

I would accept this solution.

comment:88 Changed 4 years ago by jdemeyer

My proposal would be to fix ./sage --patchbot to work both with a patchbot installed in Sage as well as a patchbot installed system-wide. This is easy to do, but it would result in what Vincent calls "a sloppy script".

comment:89 Changed 4 years ago by jdemeyer

I think the goal should be to make it as easy as possible to install and run the patchbot. We could even make it such that ./sage -i patchbot && ./sage --patchbot still works as before.

comment:90 follow-up: Changed 4 years ago by chapoton

I think it is more simple to propose a unique way to install and run the patchbot

  • install in the system python using pip
  • run using python -m

So that people have no choice to make. And this is still only two command lines.

comment:91 in reply to: ↑ 90 Changed 4 years ago by jdemeyer

Replying to chapoton:

And this is still only two command lines.

Actually, it's five commands:

  1. Download pip
  2. Install pip
  3. Download sage_patchbot
  4. Install sage_patchbot
  5. Run sage_patchbot

comment:92 Changed 4 years ago by chapoton

Meaning no offense, this comment is slightly small-minded, IMHO

comment:93 follow-up: Changed 4 years ago by embray

Let's assume for a second that users are intelligent enough to run commands on their system that are not wrapped in Sage, and that they have working systems with working Python installations...

What advantage is there to

$ ./sage -patchbot

over

$ patchbot
Last edited 4 years ago by embray (previous) (diff)

comment:94 in reply to: ↑ 93 ; follow-up: Changed 4 years ago by jdemeyer

Replying to embray:

that they have working systems with working Python installations...

A working Python installation is not the same as a working pip installation.

What advantage is there to

$ ./sage -patchbot

over

$ patchbot

The fact that ./sage --patchbot does not require the user to change his $PATH.

comment:95 in reply to: ↑ 94 Changed 4 years ago by embray

Replying to jdemeyer:

Replying to embray:

that they have working systems with working Python installations...

A working Python installation is not the same as a working pip installation.

It effectively is. pip is the official installer for Python packages and comes with modern distributions of Python (including most system distributions). While you can have a "working" Python installation without pip, a Python installation with a non-working pip installation is not a working Python installation.

What advantage is there to

$ ./sage -patchbot

over

$ patchbot

The fact that ./sage --patchbot does not require the user to change his $PATH.

Why would a user need to change their path?

comment:96 Changed 4 years ago by chapoton

  • Milestone changed from sage-7.3 to sage-7.4
  • Status changed from needs_review to needs_work

comment:97 Changed 4 years ago by jdemeyer

The sage4 patchbot is broken:

[2016-09-08 09:56:55] Getting ticket list...
Traceback (most recent call last):
  File "/var/worker.sage/local/lib/python2.7/site-packages/sage_patchbot/patchbot.py", line 1470, in main
    patchbot.test_a_ticket(ticket)
  File "/var/worker.sage/local/lib/python2.7/site-packages/sage_patchbot/patchbot.py", line 951, in test_a_ticket
    rating, ticket = self.get_one_ticket()
  File "/var/worker.sage/local/lib/python2.7/site-packages/sage_patchbot/patchbot.py", line 774, in get_one_ticket
    return all_tickets[-1]
IndexError: list index out of range

comment:98 follow-up: Changed 4 years ago by jdemeyer

There is something very wrong in how the patchbot gets its tickets from the server...

comment:99 in reply to: ↑ 98 ; follow-up: Changed 4 years ago by chapoton

Replying to jdemeyer:

There is something very wrong in how the patchbot gets its tickets from the server...

That's probably true. But your specific problem with sage4 is also that it has already recently given a report on every available ticket with "BuildFailed?" status.

comment:100 in reply to: ↑ 99 Changed 4 years ago by jdemeyer

Replying to chapoton:

But your specific problem with sage4 is also that it has already recently given a report on every available ticket with "BuildFailed?" status.

Right, that is #21430. But still, that should be completely independent from how the patchbot itself works.

comment:101 Changed 4 years ago by git

  • Commit changed from 6d7990e88c9f194f28309bd0015c40ecc09080bf to fc8bcec8a2d753fcf1a9eaf451258081cf02726b

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a3b040fSet JUPYTER_CONFIG_DIR to ~/.sage/jupyter and hardcode IPYTHONDIR
78f964eClarify comment (hopefully)
fd3ab44Update patchbot to 2.6.1 as system package
fc8bcecImprove --patchbot command

comment:102 Changed 4 years ago by jdemeyer

  • Dependencies set to #21430
  • Status changed from needs_work to needs_review

Rebased over #21430, partially squashed, fixed installations in system Python.

comment:103 Changed 4 years ago by jdemeyer

I am currently running the patchbot on sage4 and arando with this branch.

comment:104 follow-up: Changed 4 years ago by klee

If you guys are not on agreement, then I am +1 on removing or deprecating sage --patchbot. As a recent volunteer to run a patchbot, I thought it is illogical to invoke Sage to run a patchbot whose mission is to test Sage. Installing patchbot by "pip ..." and then running "python -m ..." is straightforward and simple. Why do we need an alternate and conceptually confusing way to launch a patchbot?

If this disagreement persists, then why don't we have a poll on sage-devel?

By the way, this pip install --user git+https://github.com/sagemath/sage-patchbot.git installs the latest patchbot, why does the statement in the wiki specify the version, which would be outdated sooner or later?

comment:105 in reply to: ↑ 104 ; follow-up: Changed 4 years ago by jdemeyer

Replying to klee:

If you guys are not on agreement, then I am +1 on removing or deprecating sage --patchbot. As a recent volunteer to run a patchbot, I thought it is illogical to invoke Sage to run a patchbot whose mission is to test Sage.

I don't see it as invoking Sage. I consider sage --patchbot just a simple convenience command. I changed it such that it should work in all cases: the sage_patchbot can be installed system-wide, by the user, or inside Sage. Why should we not support that?

Installing patchbot by "pip ..."

Then pip becomes an additional requirement for running the Sage patchbot. Many systems come with Python installed system-wide, but without pip.

comment:106 in reply to: ↑ 105 ; follow-ups: Changed 4 years ago by klee

I don't see it as invoking Sage. I consider sage --patchbot just a simple convenience command.

You can see it as such since you know the internals. But for those who do not know the details of how patchbot works, like me, the command gives the impression that I am about to run a patchbot installed inside Sage.

On the other hand, It is weird to control the patchbot installed in the system Python via sage interface.

comment:107 in reply to: ↑ 106 Changed 4 years ago by jdemeyer

Replying to klee:

the command gives the impression that I am about to run a patchbot installed inside Sage.

Which might be the case and which is not a problem (note however that it will always be run using the system Python).

comment:108 in reply to: ↑ 106 ; follow-up: Changed 4 years ago by embray

Replying to klee:

I don't see it as invoking Sage. I consider sage --patchbot just a simple convenience command.

You can see it as such since you know the internals. But for those who do not know the details of how patchbot works, like me, the command gives the impression that I am about to run a patchbot installed inside Sage.

On the other hand, It is weird to control the patchbot installed in the system Python via sage interface.

I agree. I think "invoking sage" here should be read as "activating the sage environment" which is true. There's no more reason to run sage --patchbot than there is to run sage --make (which is not a thing, but offers the same "convenience").

comment:109 in reply to: ↑ 108 ; follow-up: Changed 4 years ago by jdemeyer

Replying to embray:

sage --make

That option is actually called sage -i :-)

In all seriousness: I really want to support a sage_patchbot installed within Sage itself.

You may argue that this is weird. But on the other hand: why not support it? You need good arguments to explicitly not support something. I never said that this should be the only way. I do not want to force everybody to use sage --patchbot. I am only saying: it costs little to support it, so why not?

comment:110 follow-up: Changed 4 years ago by embray

Personally I see no problem with supporting sage --patchbot. Just that it should not try to run a patchbot module that's installed outside the sage environment. In other words, it should run the patchbot that's installed in sage, or do nothing at all (or at most give an error).

comment:111 in reply to: ↑ 110 Changed 4 years ago by jdemeyer

Replying to embray:

Personally I see no problem with supporting sage --patchbot. Just that it should not try to run a patchbot module that's installed outside the sage environment. In other words, it should run the patchbot that's installed in sage, or do nothing at all (or at most give an error).

That's fine for me... Frédéric, Vincent: what you do think?

comment:112 Changed 4 years ago by chapoton

Whatever way this ticket is closed, I do not care. If you manage to agree on something, go on.

comment:113 Changed 4 years ago by jdemeyer

So Frédéric, do you retract comment 85? Because then the easiest would be to go back to that branch (i.e. my branch from 76). Judging from Erik's comment, he should agree on that.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:114 Changed 4 years ago by chapoton

go on

comment:115 Changed 4 years ago by embray

Uhh, the branch at https://git.sagemath.org/sage.git/commit/?id=6d7990e88c9f194f28309bd0015c40ecc09080bf is what I'm objecting to. It attempts to run sage_patchbot with the system python as well, which I'm saying it shouldn't do. sage --whatever shouldn't touch the system Python except when building sage itself.

comment:116 Changed 4 years ago by jdemeyer

So it seems that we cannot agree on anything then...

comment:117 follow-up: Changed 4 years ago by embray

I thought we did... please re-read https://trac.sagemath.org/ticket/20736#comment:110

Keep in mind when I write "run patchbot" I'm looking at the patchbot program as a black box. I don't care that it happens to be written in Python--nor should anyone else running it. So it's rather bizarre for sage --patchbot to try to do anything with a patchbot module that is not installed in the $SAGE_LOCAL or with any python that is not in $SAGE_LOCAL

I think I see the hold-up in my own thinking. There may not be a Python in $SAGE_LOCAL.

In that case I'm back to not understanding why you even need or want sage --patchbot

Last edited 4 years ago by embray (previous) (diff)

comment:118 in reply to: ↑ 109 Changed 4 years ago by embray

Replying to jdemeyer:

You need good arguments to explicitly not support something.

I would say this is rarely true, and it is almost always the opposite that is true.

comment:119 in reply to: ↑ 117 ; follow-up: Changed 4 years ago by jdemeyer

Replying to embray:

I thought we did... please re-read https://trac.sagemath.org/ticket/20736#comment:110

Well, I want to run the pacthbot installed within Sage using the system Python. You seem to agree with the first part but not the second.

You cannot use the Python-in-Sage to run the patchbot because that one might be reinstalled: the patchbot builds Sage so it might rebuild the Python-in-Sage too. And reinstalling a program that you are running is a recipe for trouble.

comment:120 follow-up: Changed 4 years ago by embray

Okay, but when we say "the patchbot installed within Sage" you mean there's a copy of the sage_patchbot package in local/lib/python2.7/site-packages, right?

What is the advantage to doing that? site-packages is a special path as far as Python is concerned. A typical Python executable knows where to find its site-packages (well, it gets it from site.py which it knows how to find), and that site-packages in a sense "belongs" to that Python. So it's very atypical to use one Python installation to run a package that happens to be in another Python's site-packages. One exception is virtualenvs that make use of the "system site-packages" feature.

Not saying it won't work--of course ultimately it's just another directory full of Python modules one is adding to sys.path (assuming they are for the same Python version at least, which they might not be--your patch tries python2.7, followed by python2, followed by python, where the latter might be Python 2.6, or Python 3.4 or something).

TL;DR I don't see what you gain from that.

comment:121 follow-up: Changed 4 years ago by mkoeppe

I haven't followed the full discussion, nor have I ever run a patchbot, but here's my 2 cents.

+1 on using the system Python.

-1 on including the patchbot as part of the sage distribution in any way.

-1 on supporting sage --patchbot.

I think it's inviting trouble to have the testing framework depend in any way on scripts of the software to be tested.

comment:122 in reply to: ↑ 119 ; follow-up: Changed 4 years ago by rws

Replying to jdemeyer:

You cannot use the Python-in-Sage to run the patchbot because that one might be reinstalled: the patchbot builds Sage so it might rebuild the Python-in-Sage too. And reinstalling a program that you are running is a recipe for trouble.

Reinstalled by what, a Sage upgrade or a patchbot run?

comment:123 in reply to: ↑ 122 ; follow-up: Changed 4 years ago by jdemeyer

Replying to rws:

Replying to jdemeyer:

You cannot use the Python-in-Sage to run the patchbot because that one might be reinstalled: the patchbot builds Sage so it might rebuild the Python-in-Sage too. And reinstalling a program that you are running is a recipe for trouble.

Reinstalled by what, a Sage upgrade or a patchbot run?

A patchbot run (the patchbot obviously needs to build Sage, which might include a Python rebuild).

comment:124 in reply to: ↑ 120 Changed 4 years ago by jdemeyer

Replying to embray:

TL;DR I don't see what you gain from that.

I am sure I have said this already multiple times: what I gain is not requiring a system-wide pip.

I just want the requirements to run the patchbot to be as minimal as possible. With my proposal, the only requirement is Sage.

comment:125 in reply to: ↑ 123 Changed 4 years ago by rws

Replying to jdemeyer:

Replying to rws:

Replying to jdemeyer:

You cannot use the Python-in-Sage to run the patchbot because that one might be reinstalled: the patchbot builds Sage so it might rebuild the Python-in-Sage too. And reinstalling a program that you are running is a recipe for trouble.

Reinstalled by what, a Sage upgrade or a patchbot run?

A patchbot run (the patchbot obviously needs to build Sage, which might include a Python rebuild).

Just FYI: that doesn't apply to unsafe tickets (EDITED). Those get built in a sandbox under /tmp. But they don't work anyway atm.

Last edited 4 years ago by rws (previous) (diff)

comment:126 follow-up: Changed 4 years ago by rws

But wait, a Python rebuild happens potentially only with unsafe tickets, as well.

comment:127 Changed 4 years ago by rws

So your precaution doesn't matter because Python rebuilds are done in /tmp, not in the Sage installation that runs the patchbot.

comment:128 in reply to: ↑ 126 Changed 4 years ago by jdemeyer

Replying to rws:

But wait, a Python rebuild happens potentially only with unsafe tickets, as well.

Wrong. The patchbot also needs to build Sage in the usual way.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:129 Changed 4 years ago by jdemeyer

By the way, I was unaware that "unsafe" tickets were never tested. I never realized that.

comment:130 Changed 4 years ago by rws

You can test them but with some runs there were big problems. That's why it's discouraged to not use --safe-only.

comment:131 in reply to: ↑ 121 Changed 4 years ago by jdemeyer

Replying to mkoeppe:

I think it's inviting trouble to have the testing framework depend in any way on scripts of the software to be tested.

My idea is that the patchbot would not depend on anything from Sage. Only that Sage would provide convenient (but optional) scripts to run the patchbot.

comment:132 follow-up: Changed 4 years ago by klee

This ticket seems stalled because jdemeyer insists to support running patchbot installed inside Sage by the system python using Sage interface, whose idea is objected by some others.

The main objective of the ticket is to make it official to install the patchbot only as a package to the system python. This objective is in direct conflict with the jdemeyer's idea.

So my suggestion is to separate the script implementing jdemeyer's idea from the present ticket, and he opens a ticket for his script. This will facilitate closing this ticket.

comment:133 in reply to: ↑ 132 Changed 4 years ago by jdemeyer

Replying to klee:

This objective is in direct conflict with the jdemeyer's idea.

Absolutely not, there is no conflict! I just want one additional way to run the patchbot which does not harm other ways of running the patchbot.

I don't see why people are so much against an additional feature. I'm not forcing anybody to run the patchbot "my way", I just want to keep the possibility.

comment:134 Changed 4 years ago by jakobkroeker

  • Status changed from needs_review to needs_work

--sage-root=

  1. the parameter should be called --sage-testing-root= (or similar) rather than sage-root: several sage installations may exists on a system. Especially sage-root might be menthally associated with the production-sage root directory.
  1. The option sage-testing-root should be added as a config file parameter.

If supporting 'sage -patchbot' works as expected (starts the user-local or system installation of the patchbot and also uses the system python) and does not cause any serious harm (including maintenance issues) I would not insist on the complete removal of 'sage -patchbot'. A compromise between the two different opinions here could be to print a message that running 'sage -patchbot' is no longer the preferred way to run the patchbot and point to the buildbot wiki.

sloppy script

what is exactly the issue of the script by Jeroen?

Last edited 4 years ago by jakobkroeker (previous) (diff)

comment:135 Changed 4 years ago by jakobkroeker

The option sage-root should be added as a config file parameter.

@chapoton

I just discovered that it is, but in the config file the parameter name has to be sage_root (underscore instead of a dash), so here is an inconsistency. Also please update the wiki instructions https://wiki.sagemath.org/buildbot/details accordingly.

comment:136 follow-ups: Changed 4 years ago by jdemeyer

IMHO, it's obvious that the --sage-root when running the patchbot is the Sage installation that you want to test. I don't see the point of using a more obscure name like --sage-testing-root for the option.

comment:137 in reply to: ↑ 136 Changed 4 years ago by jakobkroeker

Replying to jdemeyer:

is the Sage installation that you want to test.

If I want to test a sage installation I just type './sage -t --all '

Of course you mean that 'sage-root' is here the folder where patchbot will test various tickets (and thus various sage versions)

IMHO, it's obvious that the --sage-root when running the patchbot is the Sage installation that you want to test.

Since you formally failed to correcly explain what --sage-root means, I do not buy that 'it is obvious', at least not for a new patchbot user. So I find the 'sage-root' patchbot option should be at lieast documented (what it does) on the wiki and in the software and the inconsistency between 'sage_root' and 'sage-root' should be removed (underscore vs dash)

By the way; it is well known that naming is one important part of software development and it may create mental blockades or misunderstandings.

In summary: I expect as minimal action to gain positive review that 'sage_root' gets documented in the software and in the wiki (https://wiki.sagemath.org/buildbot/details) and the inconsistency between 'sage_root' and 'sage-root' is removed (underscore in config file vs dash in command line option)

Last edited 4 years ago by jakobkroeker (previous) (diff)

comment:138 Changed 4 years ago by jakobkroeker

And the 'sage -patchbot' command should print a warning that 'sage -patchbot' is no longer the preferred way to use patchbot and print the preferred way to use the patchbot or link to the wiki (https://wiki.sagemath.org/buildbot/details)

comment:139 in reply to: ↑ 136 ; follow-up: Changed 4 years ago by embray

Replying to jdemeyer:

IMHO, it's obvious that the --sage-root when running the patchbot is the Sage installation that you want to test. I don't see the point of using a more obscure name like --sage-testing-root for the option.

Here we agree--the purpose of patchbot is testing sage installations. --sage-root is the path to the sage installation you want it to test. If there is some documentation that needs to be improved that's one thing, but putting "testing" in the option name is superfluous.

As for sage -patchbot, no there's no conflict. I'm -0.5 on it--I think it does no harm, ultimately, but it also comes across as code rot to me and doesn't serve any purpose I can glean except for "backwards compatibility", but with what I think is ultimately a misfeature anyways :/

comment:140 in reply to: ↑ 139 Changed 4 years ago by jdemeyer

Replying to embray:

doesn't serve any purpose

Well, if it didn't serve any purpose, I wouldn't insist so much. See 54 and following.

comment:141 Changed 4 years ago by embray

I'm not sure why you need to run python -m sage_patchbot.patchbot. Although that's not bad practice in cases where you want to make sure you're running the right sage_patchbot with the right python, in most cases (I think?) it suffices to just run patchbot.

So ISTM the main purpose of ./sage -patchbot is to provide a sane default for the --sage-root option. First of all, this already gets set from the SAGE_ROOT environment variable if it is set. But what if the pwd is also a valid SAGE_ROOT? Then it could take the pwd to be the default (if SAGE_ROOT is not set). That would incorporate all the convenience of running ./sage -patchbot from the same directory--more even. Compare:

$ cd my_sage
$ ./sage -patchbot
$ cd my_sage
$ patchbot

comment:142 Changed 4 years ago by git

  • Commit changed from fc8bcec8a2d753fcf1a9eaf451258081cf02726b to d7c83f1b06d1d851ef012acdf99a105cbe9db3ac

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e75e8deUpdate patchbot to 2.6.1 as system package
d7c83f1Improve --patchbot command

comment:143 Changed 4 years ago by jdemeyer

  • Dependencies #21430 deleted
  • Milestone changed from sage-7.4 to sage-7.6

Rebased to Sage-7.5.rc1

comment:144 follow-up: Changed 4 years ago by tmonteil

Since this ticket seems not to reach a consensus, what about splitting it into two part:

  • remove patchbot optional spkg, and deprecate the related sage -patchbot command (this can be done for next beta)
  • discuss and write a convenient alternative command or documentation to run the patchbot (this seems the blocking point and might require more time)

I can volonteer to implement the first point on top of the latest beta ;)

comment:145 in reply to: ↑ 144 ; follow-up: Changed 4 years ago by jdemeyer

Replying to tmonteil:

  • remove patchbot optional spkg, and deprecate the related sage -patchbot command (this can be done for next beta)
  • discuss and write a convenient alternative command or documentation to run the patchbot (this seems the blocking point and might require more time)

Doesn't "deprecate the related sage -patchbot command" relate more to the second point instead of the first?

So I would rather split up as

  1. remove patchbot optional spkg
  2. discuss how to run the patchbot and what to do with sage --patchbot

comment:146 in reply to: ↑ 145 ; follow-ups: Changed 4 years ago by tmonteil

OK, see #22473.

Replying to jdemeyer:

Doesn't "deprecate the related sage -patchbot command" relate more to the second point instead of the first?

So I would rather split up as

  1. remove patchbot optional spkg
  2. discuss how to run the patchbot and what to do with sage --patchbot

OK, I did not deprecate the command, but since the current sage -patchbot command does not work anyway, i replaced it with a short sentence pointing to https://wiki.sagemath.org/buildbot#Installing_the_patchbot (which are the most up-to-date instructions). It is just a suggestion until the current ticket get fixed, i can undo the last commit about the sage -patchbot command if required.

comment:147 in reply to: ↑ 146 Changed 4 years ago by jdemeyer

Replying to tmonteil:

It is just a suggestion until the current ticket get fixed

For the record: in my opinion, nothing needs to be fixed since nothing is broken.

comment:148 in reply to: ↑ 146 ; follow-up: Changed 4 years ago by jdemeyer

Replying to tmonteil:

https://wiki.sagemath.org/buildbot#Installing_the_patchbot (which are the most up-to-date instructions). It is just a suggestion until the current ticket get fixed

It's also an invitation to move the current discussion on this ticket to an edit war on that wiki page, so I am not so happy with that.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:149 in reply to: ↑ 148 Changed 4 years ago by tmonteil

Replying to jdemeyer:

Replying to tmonteil:

https://wiki.sagemath.org/buildbot#Installing_the_patchbot (which are the most up-to-date instructions). It is just a suggestion until the current ticket get fixed

It's also an invitation to move the current discussion on this ticket to an edit war on that wiki page, so I am not so happy with that.

OK, i removed that commit, the goal is to get rid of this broken optional package earlier, without entering the picky part of the current ticket.

comment:150 follow-up: Changed 4 years ago by embray

I think IIRC I was fine with keeping sage -patchbot as a shortcut for "run patchbot with the --sage-root option set from this sage). I think that makes sense, and I think that's what Jeroen was trying to do originally. I just thought the solution was overcomplicated.

I don't know why we're talking about python -m anything. When you install the sage-patchbot Python package (which Frédéric did a great job putting together at my urging) it installs a patchbot script. If you run that it just works, you just have to supply the correct --sage-root argument.

comment:151 Changed 4 years ago by jdemeyer

What I want in particular is to install the patchbot within the Sage environment and have sage --patchbot recognize that.

Of course, I am totally open to supporting any other use cases too (unlike some other people who explicitly do not want to suppport my use case).

comment:152 Changed 4 years ago by embray

Was there some reason that wouldn't work? That is, if you ran sage -pip install sage-patchbot? Then, in principle, it would work if you ran sage -sh -c patchbot though I guess if $SAGE_LOCAL/bin/patchbot exists then sage -patchbot can be a shortcut for that. I'm just not sure why you'd need or want to do that but I don't oppose it.

comment:153 Changed 4 years ago by jdemeyer

  • Dependencies set to #22473

comment:154 in reply to: ↑ 150 ; follow-up: Changed 4 years ago by jdemeyer

Replying to embray:

I don't know why we're talking about python -m anything. When you install the sage-patchbot Python package (which Frédéric did a great job putting together at my urging) it installs a patchbot script.

...in a directory which might not be the default $PATH (especially in the case of a --user install). So there are cases where python -m sage_patchbot.patchbot works but running patchbot does not.

comment:155 Changed 4 years ago by git

  • Commit changed from d7c83f1b06d1d851ef012acdf99a105cbe9db3ac to b423b62329b9d028a8a6de11def5fbe1eae24519

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b2810d6#22473 : remove the patchbot optional spkg.
537849ftrac 20736 first try, failing
b423b62Improve --patchbot command

comment:156 Changed 4 years ago by jdemeyer

Rebased to #22473.

comment:157 Changed 4 years ago by git

  • Commit changed from b423b62329b9d028a8a6de11def5fbe1eae24519 to c9c4101b961c454b2acac23508a74ccfbcffb7c4

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c9c4101Improve --patchbot command

comment:158 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Let's start over...

comment:159 in reply to: ↑ 154 Changed 4 years ago by embray

Replying to jdemeyer:

Replying to embray:

I don't know why we're talking about python -m anything. When you install the sage-patchbot Python package (which Frédéric did a great job putting together at my urging) it installs a patchbot script.

...in a directory which might not be the default $PATH (especially in the case of a --user install). So there are cases where python -m sage_patchbot.patchbot works but running patchbot does not.

That would imply someone has their ~/.local/lib/pythonX.Y/site-packages on sys.path, but not ~/.local/bin in their $PATH. That can certainly happen (I think it is the default in most cases) but I would consider it a slight misconfiguration. It is also not a problem at all if using virtualenvs instead.

comment:160 Changed 4 years ago by embray

  • Status changed from needs_review to positive_review

comment:161 Changed 4 years ago by jdemeyer

I wonder what made you change your mind...

This code (which has been there since 101) has been attacked by several people for 6 months and now it suddenly has positive_review?

comment:162 Changed 4 years ago by embray

My feeling at this point is along the lines of "I'm not going to use it, but if it suits your needs then fine." :)

comment:163 Changed 4 years ago by vbraun

  • Branch changed from public/20736 to c9c4101b961c454b2acac23508a74ccfbcffb7c4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.