New project item: Gdx Export - [merged] #1201

Closed spine-o-bot closed 3 years ago

In GitLab by @soininen on Sep 27, 2019, 13:28

_Merges issue#274_convert_db_togdx -> dev

This MR adds a new project item, Gdx Export to Toolbox. The item allows exporting Data Stores to GAMS .gdx files. The current implementation should give minimum support for running the TIMES model within Toolbox.

Implements #274.

What has been done here:

Added spinetoolbox.spine_io.exporters.gdx module which does the heavy lifting. Using export functionality needs GAMS and its Python bindings installed. Note that GAMS has not become a requirement and that the export functionality is merely disabled if GAMS is not found.
Added spinetoolbox.project_items.gdx_export subpackage for the project item
Some unit tests, documentation

About item naming: we discussed in the issue that perhaps this item should be called Data Export while Data Interface should correspondingly be renamed to Data Import. No renaming have been done as part of this MR as perhaps it is better to execute the renaming under a separate issue. The changes introduced here are already quite large.

To test:

Don't have GAMS installed? Fear not! You can still check if everything else works except executing the DAG and that the execution should give meaningful failure messages and not crash Toolbox.

Need to install GAMS Python bindings? Please follow the official tutorial. Note that you may need to pay attention to supported Python versions (3.6) and that both Python and GAMS have to be compiled for the same bitness (32 vs. 64).

A test database is available for download: times_mock.sqlite

Create a new project
Add a Data Store, link the above database to it
Add a Gdx Export
Connect the Data Store to the Gdx Export
Give a name to the exported file in Gdx Export's properties tab
Open the Gdx Export settings window, check that the database is represented there.
Execute the project.
If you have GAMS, open the generated .gdx there and check that sets etc. got exported in the order they were defined in the Gdx Export settings window.

Other thinks to look at:

Build the documentation, check that the updated Project items and Importing and exporting data sections are readable and the links work

In GitLab by @soininen on Oct 1, 2019, 13:08

added 2 commits

e1d32d6f - Improve gams availability check
84bd575b - Log execution start before errors

Compare with previous version

In GitLab by @soininen on Oct 1, 2019, 13:13

I do not think it is possible to run the Toolbox itself under Python 2.7.

So the Python environment that GDX Export uses should be the one that is set in Settings.

As far as I know, all the project items are executed in the same Python environment as the Toolbox. Are you proposing we sandbox all project items and execute them in an IPython console or something?

In GitLab by @ererkka on Oct 1, 2019, 13:17

I could not even install the dependencies on Python 2.7. How were you able to run the toolbox on Python 2.7?

> pip install -r requirements.txt
DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7. More details about Python 2 support in pip, can be found at https://pip.pypa.io/en/latest/development/release-process/#python-2-support
Collecting pyside2<5.12 (from -r requirements.txt (line 1))
  ERROR: Could not find a version that satisfies the requirement pyside2<5.12 (from -r requirements.txt (line 1)) (from versions: none)
ERROR: No matching distribution found for pyside2<5.12 (from -r requirements.txt (line 1))

In GitLab by @ererkka on Oct 1, 2019, 13:18

@pekka You have to install gams in the environment you are using to run the Toolbox. It’s not the same as the one in Settings --> Python interpreter.

In GitLab by @PekkaSavolainen on Oct 1, 2019, 13:19

I guess you already know the point of my test journal. We need a Gams Python Binding installation wizard built-in to Spine Toolbox. The wizard needs to automatically check the bitness of the Python that user wants to use and check that this matches the bitness of the selected GAMS and then install everything automatically.

The other option is to write a really thorough (and idiot-proof) tutorial on how to set it up.

In GitLab by @ererkka on Oct 1, 2019, 13:20

Yeah, just like for the database dialects and Julia PyCall.

In GitLab by @soininen on Oct 1, 2019, 13:22

For the moment we have a link to the official tutorial in our User Guide. Some kind of automatic setup would be nice but clearly a task under another issue.

In GitLab by @PekkaSavolainen on Oct 1, 2019, 13:24

I'm talking about this Python environment.

python_interpreter_settings

In GitLab by @PekkaSavolainen on Oct 1, 2019, 13:26

Installing Spine Toolbox on Python 2.7 has never worked. Executing Python Tools with Python 2.7 inside Spine Toolbox has worked since the support for executing Python Tools was first implemented.

In GitLab by @ererkka on Oct 1, 2019, 13:28

Right, so I was talking about a different thing... :D But anyway it does not help if you install gams for that Python 2.7.

In GitLab by @PekkaSavolainen on Oct 1, 2019, 13:31

It does not, but it should.

EDIT

Also, the GAMS program (see above Settings picture) that GDX Export uses should match the one that is selected there. (Actually, I don't know about this one yet. Don't take this seriously.)

In GitLab by @soininen on Oct 1, 2019, 13:40

I do not see why Gdx Export item should run on a specific interpreter. I believe we can require the users to install working GAMS Python bindings for the environment they are running Toolbox in. Besides, you can have multiple GAMS installations in parallel. Whatever GAMS or Python version users run their Tools in is not concern of Gdx Export. It just produces a .gdx file which the Tool can use whichever way it pleases.

In GitLab by @soininen on Oct 1, 2019, 15:52

More of a reminder to myself: agreed with @PekkaSavolainen on Skype:

[x] Use GAMS system path from Toolbox Settings when constructing GamsWorkspace
[x] Stress out the need for specific Python version and bitness in User Guide

In GitLab by @manuelma on Oct 1, 2019, 16:33

I believe tree, graph, and tabular view, are all QMainWindows. I also believe QDockWidgets only work in QMainWindows so I'm not sure we could change those for QWidgets too easily.

Maybe @PekkaSavolainen you're respecting too much the QMainWindow because of the word 'main' in it. Is that possible? I guess Spine Toolbox is also a bit awkward in the sense that it is an application, but it also has many mini-applications for which the QMainWindow is nice too...

In GitLab by @PekkaSavolainen on Oct 1, 2019, 16:40

I believe tree, graph, and tabular view, are all QMainWindows. I also believe QDockWidgets only work in QMainWindows so I'm not sure we could change those for QWidgets too easily.

True, It's too much work to change those to QWidget's without a complete overhaul of the UI. And were not doing that in the near future.

Maybe @PekkaSavolainen you're respecting too much the QMainWindow because of the word 'main' in it. Is that possible?

Yes :)

In GitLab by @soininen on Oct 2, 2019, 11:12

added 1 commit

50de3c1c - Fix .gdx export with multiple GAMS installations

Compare with previous version

In GitLab by @soininen on Oct 2, 2019, 11:14

@PekkaSavolainen I hope I got the things we discussed over Skype implemented. Could you please give this another go when you have the time?

In GitLab by @PekkaSavolainen on Oct 2, 2019, 12:58

added 1 commit

fa878199 - Updated ToolboxUI tests to take the new category item into account

Compare with previous version

In GitLab by @PekkaSavolainen on Oct 2, 2019, 13:04

added 1 commit

df611651 - GDX Export now catches a GamsException when there's a bitness mismatch.

Compare with previous version

In GitLab by @soininen on Oct 2, 2019, 13:08

There is GdxExportException specific to this module. I think we should rather raise that error instead of the very general RuntimeError here. Otherwise nice addition, thanks for working on this!

In GitLab by @soininen on Oct 2, 2019, 13:10

Further, it should be make_gams_workspace() that raises the exception.

In GitLab by @soininen on Oct 2, 2019, 13:11

The exception is in spinetoolbox.spine_io.exporters.gdx module, i.e. the same file.

In GitLab by @PekkaSavolainen on Oct 2, 2019, 13:14

ok, sorry, I thought GdxExportException is in the gams package

In GitLab by @soininen on Oct 2, 2019, 13:16

Oh no, we want to keep all gams stuff in the gdx module to isolate the rest of Toolbox from import gams issues :)

In GitLab by @PekkaSavolainen on Oct 2, 2019, 13:16

Exactly

In GitLab by @PekkaSavolainen on Oct 2, 2019, 13:18

Can you (@soininen) do the change to GdxExportException and I'll make a new comment here in the meantime?

In GitLab by @soininen on Oct 2, 2019, 13:22

Sure. I propose that in the future the reviewer requests for changes in comments and the person who opened the MR does the edits. Of course I would be very happy if someone else worked on the code but in the end I believe it keeps responsibilities clear and avoids duplicate work.

In GitLab by @PekkaSavolainen on Oct 2, 2019, 13:37

Another small thing I found out.

I have installed the GAMS Python bindings that were shipped with GAMS 28.2 (64-bit) on my Python 3.6 (64-bit). If in Spine Toolbox I go to Settings and select the GAMS 28.2 as my GAMS program, creating a GDX file works as expected. However, if I change the GAMS program in app Settings to another GAMS (64-bit), let's say GAMS 24.1, then if I try to create a GDX file again, the app exits with code 123 and the following message is printed to console.

--- Warning: The GAMS version (24.1.3) differs from the API version (28.2.0).

Ok, after writing the above, I tried this again and this time it only prints the Warning message and writes the gdx file correctly. No crash, so this a nonissue.

In GitLab by @PekkaSavolainen on Oct 2, 2019, 13:40

Don't forget to do something about the make_gams_workspace() calls in test_exporters.gdx. The tests don't work if there's a wrong GAMS in Windows registry.

In GitLab by @soininen on Oct 2, 2019, 13:42

Alright, good. My heart nearly skipped a beat there. Anyway, it is great that you have this GAMS setup that is totally bonkers. We have been able to identify quite a few issues that would have gone unnoticed on my system.

In GitLab by @PekkaSavolainen on Oct 2, 2019, 13:42

About the category name Exporting. Would Exporters be better?

In GitLab by @soininen on Oct 2, 2019, 13:45

Yes. I don't remember how I ended up with Exporting but the other option was Export Items which sounds too complicated to me. I will do the renaming.

In GitLab by @soininen on Oct 2, 2019, 13:52

Now I remember: Initially I though we would have multiple export items: Gdx Export, Excel Export, user's own MyModel export etc., hence Exporting or Export Items category. However, perhaps in the future we will rename Gdx Export to Data Export and have it support multiple output formats. With that in mind, would it make sense to rename the category already to Data Exports?

In GitLab by @PekkaSavolainen on Oct 2, 2019, 15:00

A bit of nit-picking, if I may. Export is a verb as in sell or trade abroad from thesaurus.com (different meaning in IT of course). Exporter is a noun as in merchant or something. So in our case, we will in the future group different exporter project items (GDX, Excel, among others) under the category name that is now Exporting. So in my opinion the category name should be Exporters or Data Exporters. Or something else but still a (plural) noun.

In GitLab by @manuelma on Oct 2, 2019, 15:07

I tend to agree, that Data Exporter sounds better.

In GitLab by @manuelma on Oct 2, 2019, 15:12

Yes, I deleted my comment quickly after realizing that. I think we're good. I'm actually more concerned about the Gdx in there which we'd likely remove anyways.

In GitLab by @soininen on Oct 2, 2019, 15:22

changed title from {-A n-}ew project item: Gdx Export to {+N+}ew project item: Gdx Export

In GitLab by @soininen on Oct 2, 2019, 15:25

Alright, Data Exporters it is then.

In GitLab by @PekkaSavolainen on Oct 2, 2019, 15:26

Great @soininen. Then a little note to CHANGELOG (if not already done) and I'll merge this to dev.

In GitLab by @soininen on Oct 2, 2019, 15:32

changed this line in version 12 of the diff

In GitLab by @soininen on Oct 2, 2019, 15:32

added 3 commits

0a729a66 - Replace RuntimeException by GdxExportException
cb3655c2 - Black formatting. Re #274
67de83ce - Rename 'Exporting' category to 'Data Exporters'.

Compare with previous version

In GitLab by @soininen on Oct 2, 2019, 15:33

@PekkaSavolainen CHANGELOG has been already updated so this should be good to go now.

In GitLab by @PekkaSavolainen on Oct 2, 2019, 15:40

Thanks for the hard work.

In GitLab by @PekkaSavolainen on Oct 2, 2019, 15:40

merged

In GitLab by @PekkaSavolainen on Oct 2, 2019, 15:40

mentioned in commit ff7efaec2f31bc4e15cc83d6f98c8859a3f8f5e0

In GitLab by @manuelma on Oct 2, 2019, 15:48

I don't think I wanna use it after seeing this, but it's great that it works.

spine-tools / Spine-Toolbox

New project item: Gdx Export - [merged] #1201