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

compatibility with cplex 12.9 #27790

Closed
dcoudert opened this issue May 7, 2019 · 42 comments
Closed

compatibility with cplex 12.9 #27790

dcoudert opened this issue May 7, 2019 · 42 comments

Comments

@dcoudert
Copy link
Contributor

dcoudert commented May 7, 2019

Method CPXsetlogfile has been deprecated in cplex 12.8 and removed from cplex 12.9. We must instead use CPXsetlogfilename, introduced in 12.8.

Depends on #28382

CC: @pfasante

Component: numerical

Author: David Coudert

Branch/Commit: 0881f5b

Reviewer: Sébastien Labbé

Issue created by migration from https://trac.sagemath.org/ticket/27790

@dcoudert dcoudert added this to the sage-8.8 milestone May 7, 2019
@dcoudert
Copy link
Contributor Author

dcoudert commented May 7, 2019

Commit: 62a1899

@dcoudert
Copy link
Contributor Author

dcoudert commented May 7, 2019

@dcoudert
Copy link
Contributor Author

dcoudert commented May 7, 2019

comment:1

This patch works with versions >= 12.8. However, we loose compatibility with versions < 12.8.

To maintain compatibility, we must be able to import CPXsetlogfile only if version < 12.8 and CPXsetlogfilename if version >= 12.8. Then, we can use CPXversionnumber to adjust usage.
I don't know how to do that.


New commits:

62a1899trac #27790: use CPXsetlogfilename

@videlec
Copy link
Contributor

videlec commented May 22, 2019

comment:2
  1. merge failure

  2. A possibility to make it work for both CPLEX versions, you can find out the CPLEX version from the Sage setup.py script and set a Cython constant so that you can use Cython conditional statements.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 23, 2019

Changed commit from 62a1899 to 61e5187

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 23, 2019

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

61e5187trac #27790: fix merge conflict with 8.8.beta6

@dcoudert
Copy link
Contributor Author

comment:4

Replying to @videlec:

  1. merge failure

fixed

  1. A possibility to make it work for both CPLEX versions, you can find out the CPLEX version from the Sage setup.py script and set a Cython constant so that you can use Cython conditional statements.

Not sure how to do that and where to put the code (setup.py, module_list.py ?).

Somehow, we want to extract line #define CPX_VERSION 12080000 from cpxconst.h, extract the number and use it for conditional compilation.

@videlec
Copy link
Contributor

videlec commented May 23, 2019

comment:5

Replying to @dcoudert:

Replying to @videlec:

  1. A possibility to make it work for both CPLEX versions, you can find out the CPLEX version from the Sage setup.py script and set a Cython constant so that you can use Cython conditional statements.

Not sure how to do that and where to put the code (setup.py, module_list.py ?).

It would have been easier with an optional package and a proper spkg-install... I don't know where to put it. These optional Cython modules are not very handy.

Somehow, we want to extract line #define CPX_VERSION 12080000 from cpxconst.h, extract the number and use it for conditional compilation.

I don't think that can be done directly from Cython. Here is a suggestion (similar to what is done in PyNormaliz)

  • run a configure script (or whatever) to obtain the value of CPX_VERSION and write a line "CPX_VERSION=12080000" in a file cplex_config.py
  • in the setup.py script, use the clpex_config.py to define a compile time constant and branch appropriately in the Cython code

If you find an easier way to access the CPX_VERSION macro directly from a .pyx file that would be great!

@dcoudert
Copy link
Contributor Author

comment:6

An idea to make the process transparent to users is to set the version number in module_list.py. I can get the version number like that, but I don't know how to use it with OptionalExtension.

sage: if os.path.exists(SAGE_LOCAL + '/include/cpxconst.h'):
....:     CPX_VERSION = subprocess.check_output(" grep '#define CPX_VERSION ' $SAGE_LOCAL/include/cpxconst.h | grep -o -E '[0-9]+' ", shell=True).strip()
....:     
sage: CPX_VERSION
'12080000'

@videlec
Copy link
Contributor

videlec commented May 25, 2019

comment:7

You can try the following (that works for normal extensions)

OptionalExtension(...,
   define_macros= [('CPX_VERSION', 12080000)]   # computed value
)

and the in the Cython code (with upper case)

IF CPX_VERSION >= 12080000:
    check( CPXsetlogfilename(self.env, NULL, NULL) )
ELSE:
    check( CPXsetlogfile(self.env, NULL) )

Since both versions of the code only use the self.env parameter you might want to isolate the IF/ELSE statement in a short cdef function.

@dcoudert
Copy link
Contributor Author

comment:8

This is roughly what I already tried: use define_macros, and in cplex_backend.pxd use the macro to conditionally import methods from cplex.h, but I always get

Error compiling Cython file:
------------------------------------------------------------
...
     int CPX_PARAM_SOLNPOOLGAP = 2105
     int CPX_PARAM_SOLNPOOLINTENSITY = 2107
     int CPX_MAX = -1
     int CPX_MIN = 1

IF CPX_VERSION >= 12090000:
  ^
------------------------------------------------------------

sage/numerical/backends/cplex_backend.pxd:217:3: Compile-time name 'CPX_VERSION' not defined

@videlec
Copy link
Contributor

videlec commented May 25, 2019

comment:9

try compile_time_env instead of define_macros (see cython thread)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 26, 2019

Changed commit from 61e5187 to 72e1ea0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 26, 2019

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

72e1ea0trac #27790: try to use compile time parameter

@dcoudert
Copy link
Contributor Author

comment:11

I don't understand how I can use compile_time_env.

I pushed my current trial that is not working as can be seen from the patchbot.

@videlec
Copy link
Contributor

videlec commented May 26, 2019

comment:12

line 240-241 of src/setup.py the compile_time_env variable is set up (globally). It seems that it erases any specific configuration at an Extension level.

@dcoudert
Copy link
Contributor Author

comment:13

outch... Well, either we are able to find a smart solution, or we could decide to support only cplex >= 12.8 (released Oct 2017). We already dropped support of older versions when the API introduced file cpxconst.h (#11958).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 30, 2019

Changed commit from 72e1ea0 to 93ff118

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 30, 2019

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

93ff118trac #27790: another trial

@dcoudert
Copy link
Contributor Author

comment:15

still not working :(

@dcoudert
Copy link
Contributor Author

comment:17

I'm more and more in favor of dropping support for cplex versions < 12.8. Indeed, we have an easy solution to support versions >= 12.8, but no simple, and more important working, solution for conditional compilation enabling to support < 12.8.

@dcoudert dcoudert modified the milestones: sage-8.8, sage-8.9 Jun 17, 2019
@dcoudert
Copy link
Contributor Author

comment:18

another argument for dropping support of version < 12.8: the IBM academic initiative allows to get versions 12.8 and 12.9, and no prior version.

@dcoudert
Copy link
Contributor Author

dcoudert commented Oct 8, 2019

comment:19

We should either wait for #28382 or rebase this ticket on it as #28382 correct the types used in this backend.

@dcoudert dcoudert added this to the sage-9.0 milestone Oct 8, 2019
@seblabbe
Copy link
Contributor

seblabbe commented Nov 8, 2019

comment:20

I get the error below when I try to make sage 9.0.beta4 with the current branch:

[sagelib-9.0.beta4] [1/1] Cythonizing sage/numerical/backends/cplex_backend.pyx
[sagelib-9.0.beta4] /home/slabbe/GitBox/sage/local/lib/python2.7/site-packages/Cython/Compiler/Main.py:369: FutureWarning: Cython directive 'language_level' not set, using 2 for now (Py2). This will change in a later release! File: /home/slabbe/GitBox/sage/src/sage/numerical/backends/cplex_backend.pxd
[sagelib-9.0.beta4]   tree = Parsing.p_module(s, pxd, full_module_name)
[sagelib-9.0.beta4] Discovering Python/Cython source code....
[sagelib-9.0.beta4] Discovered Python/Cython sources, time: 0.02 seconds.
[sagelib-9.0.beta4] running build
[sagelib-9.0.beta4] Generating auto-generated sources
[sagelib-9.0.beta4] Building interpreters for fast_callable
[sagelib-9.0.beta4] running build_cython
[sagelib-9.0.beta4] Enabling Cython debugging support
[sagelib-9.0.beta4] Updating Cython code....
[sagelib-9.0.beta4] ************************************************************************
[sagelib-9.0.beta4] Traceback (most recent call last):
[sagelib-9.0.beta4]   File "setup.py", line 871, in <module>
[sagelib-9.0.beta4]     ext_modules = ext_modules)
[sagelib-9.0.beta4]   File "/home/slabbe/GitBox/sage/local/lib/python2.7/distutils/core.py", line 151, in setup
[sagelib-9.0.beta4]     dist.run_commands()
[sagelib-9.0.beta4]   File "/home/slabbe/GitBox/sage/local/lib/python2.7/distutils/dist.py", line 953, in run_commands
[sagelib-9.0.beta4]     self.run_command(cmd)
[sagelib-9.0.beta4]   File "/home/slabbe/GitBox/sage/local/lib/python2.7/distutils/dist.py", line 972, in run_command
[sagelib-9.0.beta4]     cmd_obj.run()
[sagelib-9.0.beta4]   File "setup.py", line 770, in run
[sagelib-9.0.beta4]     build.run(self)
[sagelib-9.0.beta4]   File "/home/slabbe/GitBox/sage/local/lib/python2.7/distutils/command/build.py", line 127, in run
[sagelib-9.0.beta4]     self.run_command(cmd_name)
[sagelib-9.0.beta4]   File "/home/slabbe/GitBox/sage/local/lib/python2.7/distutils/cmd.py", line 326, in run_command
[sagelib-9.0.beta4]     self.distribution.run_command(command)
[sagelib-9.0.beta4]   File "/home/slabbe/GitBox/sage/local/lib/python2.7/distutils/dist.py", line 972, in run_command
[sagelib-9.0.beta4]     cmd_obj.run()
[sagelib-9.0.beta4]   File "setup.py", line 323, in run
[sagelib-9.0.beta4]     cache=False,
[sagelib-9.0.beta4]   File "/home/slabbe/GitBox/sage/local/lib/python2.7/site-packages/Cython/Build/Dependencies.py", line 966, in cythonize
[sagelib-9.0.beta4]     aliases=aliases)
[sagelib-9.0.beta4]   File "/home/slabbe/GitBox/sage/local/lib/python2.7/site-packages/Cython/Build/Dependencies.py", line 808, in create_extension_list
[sagelib-9.0.beta4]     raise TypeError(msg)
[sagelib-9.0.beta4] TypeError: pattern is not of type str nor subclass of Extension (<class distutils.extension.Extension at 0x7fc827d040b8>) but of type <type 'list'> and class <type 'list'>
[sagelib-9.0.beta4] ************************************************************************
[sagelib-9.0.beta4] Error building the Sage library
[sagelib-9.0.beta4] ************************************************************************
[sagelib-9.0.beta4] Please email sage-devel (http://groups.google.com/group/sage-devel)
[sagelib-9.0.beta4] explaining the problem and including the relevant part of the log file
[sagelib-9.0.beta4]   /home/slabbe/GitBox/sage/logs/pkgs/sagelib-9.0.beta4.log
[sagelib-9.0.beta4] Describe your computer, operating system, etc.
[sagelib-9.0.beta4] ************************************************************************

@dcoudert
Copy link
Contributor Author

dcoudert commented Nov 8, 2019

comment:21

The current branch is not working. It's an unsuccessful trial to detect cplex version at compile time. I will push a new branch soon.

@dcoudert
Copy link
Contributor Author

dcoudert commented Nov 8, 2019

@dcoudert
Copy link
Contributor Author

dcoudert commented Nov 8, 2019

Dependencies: #28382

@dcoudert
Copy link
Contributor Author

dcoudert commented Nov 8, 2019

Changed commit from 93ff118 to abd00fe

@dcoudert
Copy link
Contributor Author

dcoudert commented Nov 8, 2019

comment:22

I pushed a new branch, built on top of 9.0.bet4 and #28382 (but not #28708, so corresponding doctests are still here).

The changes are in file cplex_backend.pxd

-     # sets the log stream file
-     int CPXsetlogfile (c_cpxlp * env, FILE * f)
+     # Set the log file
+     int CPXsetlogfilename(c_cpxenv * env, char * filename, char * mode)

and in file cplex_backend.pyx

-        cdef FILE *ff
         if name.lower() == "logfile":
             if value is None: # Return logfile name
                 return self._logfilename
             elif not value:   # Close current logfile and disable logs
-                check( CPXsetlogfile(self.env, NULL) )
+                check( CPXsetlogfilename(self.env, NULL, NULL) )
                 self._logfilename = ''
             else:             # Set log file to logfilename
-                ff = fopen(str_to_bytes(value), "a")
-                if not ff:
-                    raise ValueError("Unable to append file {}.".format(value))
-                check( CPXsetlogfile(self.env, ff) )
+                check( CPXsetlogfilename(self.env, str_to_bytes(value), "a") )
                 self._logfilename = value

The drawback of this branch is to loose compatibility with cplex versions prior to 12.8, but since it is more than 2 years old and that the current version is 12.9, this is certainly OK.
May be we should add something in http://doc.sagemath.org/html/en/thematic_tutorials/linear_programming.html ?


New commits:

94026d1trac #28382: fix compilation warnings with cplex backends
f1b2279trac #27790: Merged 28382 with
abd00fetrac #27790: use CPXsetlogfilename instead of CPXsetlogfile

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 9, 2019

Changed commit from abd00fe to 37dcc4d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 9, 2019

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

37dcc4dtrac #27790: update thematic tutorial on linear programming

@dcoudert
Copy link
Contributor Author

dcoudert commented Nov 9, 2019

comment:24

Some updates to the thematic tutorial: license file no longer needed and make it clear that we drop support fo versions < 12.8

@seblabbe
Copy link
Contributor

seblabbe commented Nov 9, 2019

Reviewer: Sébastien Labbé

@seblabbe
Copy link
Contributor

seblabbe commented Nov 9, 2019

comment:25

Works for me with both 12.8 and 12.9.

I agree to drop support for versions < 12.8.

@dcoudert
Copy link
Contributor Author

dcoudert commented Nov 9, 2019

comment:26

Thank you so much for the great help. I will finally be able to use cplex 12.9 without patch :))

@seblabbe
Copy link
Contributor

comment:27

Great! I am sorry I took so long to come by and review your tickets.

@vbraun
Copy link
Member

vbraun commented Nov 14, 2019

comment:28

Merge conflict

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 18, 2019

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

0881f5btrac #27790: fix merge conflict with 9.0.beta6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 18, 2019

Changed commit from 37dcc4d to 0881f5b

@dcoudert
Copy link
Contributor Author

comment:30

I fixed the merge conflicts with last beta.

@dcoudert
Copy link
Contributor Author

comment:32

Thanks !

@vbraun
Copy link
Member

vbraun commented Nov 24, 2019

Changed branch from public/linear_programming/27790_cplex to 0881f5b

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

No branches or pull requests

4 participants