-
-
Notifications
You must be signed in to change notification settings - Fork 399
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
[WIP] Implementation of write_to_file that use MathOptFormat. #1982
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start. I added comments on some technical points that need to be addressed before merging.
src/JuMP.jl
Outdated
""" | ||
writeLP(model::Model, fname::AbstractString) | ||
|
||
Exports the input model as a LP file. To compress the output file, use a `.gz` extension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which LP format? There are at least two formats called LP:
http://lpsolve.sourceforge.net/5.0/CPLEX-format.htm
http://lpsolve.sourceforge.net/5.1/lp-format.htm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CPLEX (although there are outstanding issues re naming odow/MathOptFormat.jl#67).
src/JuMP.jl
Outdated
function writeLP(model::Model, fname::AbstractString) | ||
lp_model = MathOptFormat.LP.Model() | ||
MOI.copy_to(lp_model, backend(model)) | ||
MOI.write_to_file(lp_model, fname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do the JuMP variable names translate to variable names in the LP file? What happens if multiple variables have the same (or empty) names? What if the variable name is incompatible with LP format, e.g., "5 * x2 + x3"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open issue: odow/MathOptFormat.jl#67
There is code in LPWriter.jl to sanitize the names, but I haven't ported it yet.
src/JuMP.jl
Outdated
end | ||
|
||
""" | ||
writeMPS(model::Model, fname::AbstractString) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments as for write_LP
.
Thanks for your comments! I've adapted the code as you suggested; as far as I can see, your other remarks are more about the documentation than the code, I will handle them when I have more time for it :)! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could potentially just have
function write_to_file(model::Model, filename::String)
and dispatch to the appropriate MathOptFormat
model based on the extension.
src/JuMP.jl
Outdated
""" | ||
writeLP(model::Model, fname::AbstractString) | ||
|
||
Exports the input model as a LP file. To compress the output file, use a `.gz` extension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CPLEX (although there are outstanding issues re naming odow/MathOptFormat.jl#67).
Having a simpler By the way, how expensive is the copy operation for large models? Couldn't we use the solvers' functionalities for writing models to files in order to avoid a copy? We would then have to deal with the variable syntax of LP formats, for instance (how about a keyword argument to choose the variant, resulting in either the solver being called, or MathOptFormat coming into play, or an error being thrown?). (How about giving access to more "advanced" solver file outputs, such as solution files, IISes, or even bases, solution pools? I suspect these are not available through MOI right now, but could be highly important in some applications.) EDIT: if the operations are handed to the solver, then it should know the name of the variables (when I looked at it, it seems the names are kept in JuMP and not passed over to the solver). Probably related to jump-dev/CPLEX.jl#225. |
Performance-wise, the current code is a tad slow for my needs: to output 779 models (a total of 17 MiB of data), I need roughly 25 seconds on my under-powered machine (Intel i7 6600U), while solving all these problems only takes 2.5 seconds with CPLEX (roughly, 50 constraints, 150 variables each). I never saw such slowdowns with previous versions of JuMP (pre-MOI) just due to outputting LP files. |
It would be useful to know what proportion of the slow-down was in copying the models from JuMP to If it's the former, switching from LQOI to native backends (e.g., jump-dev/Gurobi.jl#216) might help. If it's the latter, then we should investigate improvements in MathOptFormat. |
Bump. MathOptFormat is now registered. Did you take a look at the performance issue? |
Unfortunately, not yet, although I've been thinking of it recently. I'll try to do something soon. |
First, just to compare the cost of exporting LPs with respect to solving them, here is what I got (one pair of lines per experiment I ran, which is mostly solving a large amount of small LPs with CPLEX, just continuous programs so no B&B stuff) -- just to note, I did not add any kind of loop to compute the timings multiple times:
Then, with more details, I split the LP timings into three part: creating a new
It seems like the majority of the time spent is due to writing things to disk. Just to be complete, I ran the tests on my Windows 10 x64 1809 laptop (with an SSD!).
|
Do you have the code so I can run? What is LPS and OPT? |
I can send you the code privately, if you like (which email address?). Otherwise: LPS for the complete time to write down the LP files; OPT for the duration of the calls to |
Can you post a gist of a simplified benchmark? Otherwise oscar.dowson at northwestern.edu |
I used the integrated profiler to generate those files (both tree and flat profiles). The procedure: first start my code (with an include), then import Profile, restart the code ( In the meantime, I suppose the code could be merged as is, with performance improvements coming in future iterations? |
Instead of copying the old JuMP syntax, how about something like: @enum(FileFormats, MPS, LP, MOF)
function write_to_file(model::Model, io::IO; format::FileFormats = MPS, kwargs...)
dest = if format == MPS
MathOptFormat.MPS.Model(; kwargs...)
elseif format == LP
MathOptFormat.LP.Model(; kwargs...)
else
@assert format == MOF
MathOptFormat.MOF.Model(; kwargs...)
end
MOI.copy_to(dest, backend(model))
MOI.write_to_file(dest, io)
return
end
"""
write_to_file(model::Model, filename::String; format = MPS, kwargs...)
Write `model` to the file called `filename` using the format `format`.
Valid formats are given by the enum [`FileFormats`](@ref).
For keyword options, see [MathOptFormat.jl](https://github.com/odow/MathOptFormat.jl).
"""
function write_to_file(model::Model, filename::String; format = MPS, kwargs...)
open(filename, "w") do io
write_to_file(model, io; format = format, kwargs...)
end
return
end This PR also needs tests and documentation before we can merge. |
This other API seems nice; to make it as convenient to use as the old one, what about detecting the format from the extension (if no keyword argument provides it)? The same could be used for compression (detection of an extension, plus another keyword argument to overwrite the extension). |
We can actually hook into the compression logic in MathOptFormat (https://github.com/odow/MathOptFormat.jl/blob/39048ec3d50153b38992f8a6b38a60396fa3433a/src/MathOptFormat.jl#L143-L157). """
write_to_file(model::Model, io::Union{IO, String}; format = MPS, kwargs...)
Write `model` to `io` using the format `format`.
If `io` is an `IO`, then the model will be written to that IO stream.
If `io` is a `String`, then the model will be written to the to file
called `io`. If `io` is a `String` that ends in `.gz`, then the file
will be compressed using GZip.
Valid formats are given by the enum [`FileFormats`](@ref).
For keyword options, see [MathOptFormat.jl](https://github.com/odow/MathOptFormat.jl).
"""
function write_to_file(
model::Model, io::Union{IO, String};
format::FileFormats = MPS, kwargs...
)
dest = if format == MPS
MathOptFormat.MPS.Model(; kwargs...)
elseif format == LP
MathOptFormat.LP.Model(; kwargs...)
else
@assert format == MOF
MathOptFormat.MOF.Model(; kwargs...)
end
MOI.copy_to(dest, backend(model))
MOI.write_to_file(dest, io)
return
end |
You could have the format be |
02008a7
to
7c66b75
Compare
Here is what I came up to. Besides rebasing and implementing the proposed changes, I also added an enumeration for compression (in case you want the GZip compression, but have other requirements for the extension). I think parts of this should move into MathOptFormat, though. What is your opinion? (By the way, the failure seems related to a wrong UUID for MOF.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are failing. Do you need to add a [compat]
entry for MOF?
You might want to rename export.jl
to file_formats.jl
. It could get confused with Julia's export
keyword.
Actually, I have not really tested this code, I was more looking for feedback on the proposed API (including the split between JuMP and MOF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you looked at
https://github.com/odow/MathOptFormat.jl/blob/e2041074fa6080f00bfb8bf290ebff84f3afbe8e/src/MathOptFormat.jl#L136-L162
CodecZlib is already included in MathOptFormat.
You can probably drop the compression
argument, and just infer from the filename.
You really just need
function write_to_file(model, filename; format, kwargs...)
dest = if format == AUTOMATIC_FILE_FORMAT
_filename_to_format(filename)
elseif format == CBF
MathOptFormat.CBF.Model(; kwargs...)
elseif ...
...
end
MOI.copy_to(dest, backend(model))
MOI.write_to_file(dest, filename)
end
Codecov Report
@@ Coverage Diff @@
## master #1982 +/- ##
==========================================
- Coverage 91.01% 90.36% -0.66%
==========================================
Files 40 39 -1
Lines 4350 4347 -3
==========================================
- Hits 3959 3928 -31
- Misses 391 419 +28
Continue to review full report at Codecov.
|
I'd be in favor of moving the compression logic into MathOptFormat. |
I actually took inspiration from that part of MOF's code :)! I also included support for more than just GZip compression (which does not always do that great), but that should belong to MOF, I think. |
The compression logic is proposed in odow/MathOptFormat.jl#90; the latest commit is based on this other PR. |
* Move most of the code from jump-dev/JuMP.jl#1982. * Refactor to use a dictionary storing all the variance within compression codecs. * Same refactoring for model types. * Start restructuring compression format handling. * Finalise implementation based on types. * Add documentation for AbstractCompressionScheme. * Clean up code. * Add more tests. * Comment out XZ. * Remove dependency for commented out code. * Comment out XZ.
a8df44d
to
6991b86
Compare
With MOF master, tests pass (locally). |
Did you forget to push some commits? The code you have isn't tested, and it won't run because I removed You probably want something like: function write_to_file(
model::Model,
filename::String;
format::MathOptFormat.FileFormat = MathOptFormat.AUTOMATIC_FILE_FORMAT,
compression::MathOptFormat.AbstractCompressionScheme =
MathOptFormat.AutomaticCompression(),
)
if file_format == AUTOMATIC_FILE_FORMAT
file_format = _detect_file_format(filename)
end
dest = MathOptFormats._FILE_FORMATS[file_format][2]()
MOI.copy_to(dest, backend(model))
MOI.write_to_file(dest, filename; compression = compression)
return
end |
Follows odow/MathOptFormat.jl#70. Not mergeable right now due to MOF not being registered (but it may happen soon, as I understood).