tansial commented 5 years ago

I downloaded version 4.0.2 and started testing. There are links in the model graph that will return the "Not Found" error. I didn't find a correlation for the ones that are not working, you can find non-working links in many different subsections.

Example: core -> CO2MPAS model -> calculate_precondition_output -> after_treat_model

--->

but already CO2MPAS model clickable link returns the error. It is the same if I open from the co2wui or the consolle.

Furthermore, I see some clickable items I didn't expect to be there: bypass, selector, domain... Are they supposed to be there?

ankostis commented 5 years ago

@vinci1it2000 any idea?

dimitriskomnos commented 5 years ago

This does not work in the console either, should not we transfer in co2mpas?

[Edit] speaking with @tansial, he confirmed that this works from console

stefanocorsi commented 5 years ago

@tansial, @dimitriskomnos and @vinci1it2000 it works for me from wui and console, though instead calculate precondition output does not work both from co2wui and from console. I get this error message when I kill the process:

from console:

2019-10-05 09:23:52,909:ERROR:schedula.utils.drw:dot could not render F:\dati\personale\jrc\co2mpas\co2wui.git\cache_plot\static\core/CO2MPAS_model/calculate_precondition_output.html due to:
 CalledProcessError(3221225786, ['dot', '-Tsvg', '-O', 'tmp61dcgzim'])

and

from co2wui:

dot could not render F:\dati\personale\jrc\co2mpas\co2wui.git\cache\static\core/CO2MPAS_model/calculate_precondition_output.html due to:
 CalledProcessError(3221225786, ['dot', '-Tsvg', '-O', 'tmp48t6qe5p'])
dot could not render F:\dati\personale\jrc\co2mpas\co2wui.git\cache\static\core/CO2MPAS_model/calculate_precondition_output.html due to:
 CalledProcessError(3221225786, ['dot', '-Tsvg', '-O', 'tmpubrkm25f'])
dot could not render F:\dati\personale\jrc\co2mpas\co2wui.git\cache\static\core/CO2MPAS_model/calculate_precondition_output.html due to:
 CalledProcessError(3221225786, ['dot', '-Tsvg', '-O', 'tmpaox316ry'])
dot could not render F:\dati\personale\jrc\co2mpas\co2wui.git\cache\static\core/CO2MPAS_model/calculate_precondition_output.html due to:
 CalledProcessError(3221225786, ['dot', '-Tsvg', '-O', 'tmpn0vhnf09'])

vinci1it2000 commented 5 years ago

@stefanocorsi should be the problem of the exe that doesn’t include the graphviz library. I’m moving to the forge.

vinci1it2000 commented 5 years ago

@tansial regarding the bypass, the name is normal because is the function used inside the model.

ankostis commented 5 years ago

I'm sure Graphvix is included, because last time i checked it (June) it was in the client Vagrant-VM, which does not include Graphviz (check the code).

Besides, it wouldn't work for all diagrams, not just for some of them, towards the bottom, correct?

ankostis commented 5 years ago

@vinci1it2000 @stefanocorsi i've been thinking that we can improve a lot the way graphs are rendered. Writing files into disk and serving them with an ad hoc flask-server seems like a waste of magnetic spin....a slickier approach would be to use an all-javascript solution for rendering the graphs in browser's <canvas> element, eg the Javascript Infovis Toolkit. And we gain no server, no back and forth with browser buttons, interactivity.

Other alternatives are D3.js & bokeh, but there are curated list of tools for that:

stefanocorsi commented 5 years ago

@ankostis that seems a great idea. Next release?

ankostis commented 5 years ago

Certainly!

stefanocorsi commented 5 years ago

Some data/evidence gathered today for @vinci1it2000 and @ankostis in order to help me diagnose the causes of this issue:

I launched a virtual machine windows 10
opened cmd and tried to run "dot" -> not found
installed Co2mpas executable version
opened a normal cmd window and tried to run "dot" -> not found
ran the co2mpas console window
in the console tried to run "dot" -> works!!! (so graphviz is installed)
launched co2wui
launched model plot
navigated to co2mpas model -> not found http://127.0.0.1:49900/core/CO2MPAS_model.html

got the following error message from server console:

dot could not render C:\Users\vagrant\Documents\cache\static\core/CO2MPAS_model.html due to:
CalledProcessError(2, ['dot.bat', '-Tsvg', '-O', 'tmpsyls4_oc'])

tried to find C:\Users\vagrant\Documents\cache\static\core/CO2MPAS_model.html on the file system -> not found
tried to find tmpsyls4_oc in C:\Users\vagrant\Documents\cache\static\core -> found

output of conda list | grep graph

(base) C:\Users\vagrant\Documents>conda list | grep graph
cryptography              2.7              py37hb32ad35_0    conda-forge
graphviz                  2.38.0            h6538335_1011    conda-forge
python-graphviz           0.13                       py_0    conda-forge

Conclusions:

graphviz is installed by the executable installer and is callable in the conda environment of the console and of the server
the graph file (e.g. tmpsyls4_oc) exists and contains valid graphviz commands
the html file of the model diagram seems not to exist

At this point, two possible causes for the issue we are discussing come to my mind:

a) The flask server launched by the plot does not inherit or does not set the correct PATH environment b) There's a problem with the paths of the generated html files

sapofra commented 5 years ago

Here the error message when I try to plot.

Co2mpas GUI application is now running on port 53452.
A browser should have been automatically launched with the correct address
If you aren't redirected automatically, please point your browser to:
    http://localhost:53452
'\\?\C:\Users\sapofra\Documents\cache\static'
CMD.EXE was started with the above path as the current directory.
UNC paths are not supported.  Defaulting to Windows directory.
Error: dot: can't open tmp2vkfhfzh
dot could not render C:\Users\sapofra\Documents\cache\static\core.html due to:
 CalledProcessError(2, ['dot.bat', '-Tsvg', '-O', 'tmp2vkfhfzh'])

stefanocorsi commented 5 years ago

@vinci1it2000 I think I have a clue regarding this issue. When I start "co2mpas plot" from the console provided by the executable installer, I get this message:

(base) C:\Users\vagrant\Documents>co2mpas plot
 * Serving Flask app "C:\Users\vagrant\Documents\cache_plot" (lazy loading)
 * Environment: production
   WARNING: This is a development server. Do not use it in a production deployment.
   Use a production WSGI server instead.
 * Debug mode: off
2019-10-12 09:34:12,666: INFO:werkzeug: * Running on http://127.0.0.1:5000/ (Press CTRL+C to quit)
2019-10-12 09:34:13,338: INFO:co2mpas:Done! [17.17 sec]
2019-10-12 09:34:14,978: INFO:werkzeug:127.0.0.1 - - [12/Oct/2019 09:34:14] "GET / HTTP/1.1" 200 -
2019-10-12 09:34:16,510: INFO:werkzeug:127.0.0.1 - - [12/Oct/2019 09:34:16] "GET /favicon.ico HTTP/1.1" 404 -
2019-10-12 09:34:16,556: INFO:werkzeug:127.0.0.1 - - [12/Oct/2019 09:34:16] "GET /favicon.ico HTTP/1.1" 404 -
'\\?\C:\Users\vagrant\Documents\cache_plot\static'
CMD.EXE was started with the above path as the current directory.
UNC paths are not supported.  Defaulting to Windows directory.
Error: dot: can't open tmpdx48cqh0
2019-10-12 09:34:29,556:ERROR:schedula.utils.drw:dot could not render C:\Users\vagrant\Documents\cache_plot\static\core.html due to:
 CalledProcessError(2, ['dot.bat', '-Tsvg', '-O', 'tmpdx48cqh0'])
2019-10-12 09:34:29,556: INFO:werkzeug:127.0.0.1 - - [12/Oct/2019 09:34:29] "GET /core.html HTTP/1.1" 404 -

I would concentrate on this part of the message:

'\\?\C:\Users\vagrant\Documents\cache_plot\static'
CMD.EXE was started with the above path as the current directory.
UNC paths are not supported.  Defaulting to Windows directory.
Error: dot: can't open tmpdx48cqh0

I wonder if somewhere in the schedula code, UNC paths are used to be passed on to the dot intepreter... In particular I'm looking at this code: https://github.com/vinci1it2000/schedula/blob/d3796d5ae1845d634bfc89f138c0ef80003dcba6/schedula/utils/drw/__init__.py#L53

_UNC = u'\\\\?\\' if PLATFORM == 'windows' else ''

In utils.py in schedula.

I'm doing some tests now removing the line and also investigating what this means for windows and multiplatform usage.

stefanocorsi commented 5 years ago

@vinci1it2000 update: it works when the line is changed to this:

_UNC = ''

ankostis commented 5 years ago

(I know it's not my business..) @vinci1it2000 why is cmd-shell involved?

ankostis commented 5 years ago

Issue reported on vinci1it2000/schedula#16.

For this release, any fix there would suffice (@stefanocorsi a newschedula release may require a new rebuild of the EXE).

For the next release, i would like to see a "decoupled" solution, where schedula functions as a library alone and not launching its own server to render plots - it can provide its own flak rules/blueprint though.

vinci1it2000 commented 5 years ago

It is decoupled.

https://github.com/vinci1it2000/co2mpas/blob/dfcc06bd7bd31337dfb16efa4b93394235b6822b/co2mpas/__init__.py#L199

You can overwrite the flask parameters using the kwargs of the run method. Or if you want the app replace run with app.

vinci1it2000 commented 5 years ago

@ankostis why there is a CMD.EXE invoked? This should be your business.

@stefanocorsi anyway we don't need any new release of schedula. When co2mpas is launched from CMD.EXE you can use the following patch:

import schedula.utils.drw as mdl
setattr(mdl, '_UNC', '')

stefanocorsi commented 5 years ago

Is it possible that setting UNC to '' will create problems to long paths (limit 256 chars)?

vinci1it2000 commented 5 years ago

That problem persist.

ankostis commented 5 years ago

@ankostis why there is a CMD.EXE invoked? This should be your business.

I don't know where (or if) cmd .exe comes is involved. co2wui simply calls the co2mpas dispatcher to launch the graph-plots flask-server. It's the error-message Stefano pasted that make me suspect that cmd.exe it is involved in the execution of your flask-server.

It is decoupled.

https://github.com/vinci1it2000/co2mpas/blob/dfcc06bd7bd31337dfb16efa4b93394235b6822b/co2mpas/__init__.py#L199

You can overwrite the flask parameters using the kwargs of the run method. Or if you want the app replace run with app.

With "decoupling" i meant the opposite:

Instead of co2wui getting the graph-plots Flask.app from schedula, we want co2wui to provide the app, and schedula to populate it with URL flask-rules. Anothe alternative it for schedula (sitemap?) to provide a flask blueprint which contains the rules, and co2wui to use it when launching the base flask-server.

In essence, schedula must not start up any server.

That problem persist.

Please discuss in vinci1it2000/schedula#16 how to make it less persistent.

spycon69 commented 5 years ago

@vinci1it2000 do you need further clarifications in regards to @ankostis request above ? This is a top-priority issue so we need to fix asap. Please clarify if u need any further assistance.

ankostis commented 5 years ago

To clarify, we don't want the "decoupled" solution now.

We need to know if the proposed monkeypatch to drop UNCs will affect the supported path-lengths; co2mpas have some deeply nested graphs that exceed the 260-char limit of non-UNC paths (accounting also the installation-dir).

vinci1it2000 commented 5 years ago

@spycon69 I said that the issue is not schedula. The problem is CMD.EXE that doesn't allow to use UNC paths and the cmd dot cannot render a file (both problems belong to the jrcstu-forge, for this reason, I opened an issue there https://github.com/JRCSTU/jrcstu-forge/issues/2 ). I cannot solve this issue because I'm not involved in the development of the exe and I have no clue why the console is launched with the CMD.EXE. This is a decision made by @ankostis and @stefanocorsi.

vinci1it2000 commented 5 years ago

@ankostis regarding your previous https://github.com/JRCSTU/co2wui/issues/146#issuecomment-541641361. The alternative is to iterate over the attribute of the Flask app url_map and view_functions and take rule, endpoint, view_func. The latest three parameters are needed to construct the flask app. In this way, you have a complete decoupled solution.

ankostis commented 5 years ago

Vinci, you didn't notice that the problem affects even also the co2mpas plot command-line tool. Unless i'm missing something fundamental here, this has nothing to do with co2wui or conda for that matter, no? Are we supposed to run co2mpas from PowerShell now?

Additionally, it is schedula calling Graphviz process and feeding it the DOT files which schedula has saved in deep nested folder, correct? So it seems that is is not related to "which console we use to launch the co2mpas plot command", but rather related to the subprocess you launch the DOT file processor(Graphviz), correct? In all this, we have no intervention, since everything is behind a dispatcher.

Finally, and most fundamentally, why you had to provide a "dispatcher API" for plain tasks like this (and i do't mean the actual graph processing & rendering, which by definition they are a graph and fit perfectly fine with schedula, but the flask-app/server thing)? You really make debugging hard as hell, and you know, the lives of fellow developers suffer when we cannot debug freely :-)

C:\Users\vagrant\Documents>co2mpas plot
* Serving Flask app "C:\Users\vagrant\Documents\cache_plot" (lazy loading)
...

Regarding the decoupling, this seems like another hack, which i'm not sure it would work in all occasions. Wouldn't it be better if you retrofitted schedula to provide a Flask blueprint, which are made with this use-case in mind?

vinci1it2000 commented 5 years ago

Yes, you missed one stuff. This error appears only on the exe. I cannot reproduce on my window. This problem belong to the jrcstu-forge.

Additionally, it is schedula calling Graphviz process and feeding it the DOT files which schedula has saved in deep nested folder, correct?

Yes.

So it seems that is is not related to "which console we use to launch the co2mpas plot command", but rather related to the subprocess you launch the DOT file processor(Graphviz), correct?

No. You can read the first answare.

Finally, and most fundamentally, why you had to provide a "dispatcher API" for plain tasks like this (and i do't mean the actual graph processing & rendering, which by definition they are a graph and fit perfectly fine with schedula, but the flask-app/server thing)?

You know why. The rendering of the model graph takes time so with the API you have the plot generated ondemand.

Regarding the decoupling, this seems like another hack, which i'm not sure it would work in all occasions. Wouldn't it be better if you retrofitted schedula to provide a Flask blueprint, which are made with this use-case in mind?

I cannot understand why you want the flask bluprint and how it can solve the problem with the UNC paths, CMD.EXE, and DOT.

ankostis commented 5 years ago

you missed one stuff. This error appears only on the exe.

@stefanocorsi @dimitriskomnos @sapofra is this true?

I cannot reproduce on my window.

Use the Vagrant VM then.

So it seems that is is not related to "which console we use to launch the co2mpas plot command", but rather related to the subprocess you launch the DOT file processor(Graphviz), correct?

No. You can read the first answare.

~~But then why you say it's jrcstu-forge problem?~~[edit: i undestood the wrong target meaning for "no"]

I cannot understand why you want the flask bluprint

Because that is why flask-blueprints exist.

On the contrary, from a quick search I could not find any similar recipe for transplanting flask rules. (btw, to get them you don't iterate over all app's attributes but there is iter_rules method).

[I cannot understand] how it can solve the problem with the UNC paths, CMD.EXE, and DOT.

It won't. It would make the architecture simpler, to debug bugs like that.

dimitriskomnos commented 5 years ago

In the jrc desktop in the release 4.1.1 it worked in the console and it was giving error in the WUI. In my laptop both WUI and console give me the same error.

Also, in the jrc desktop I tried with the downloaded projects the latest versions of co2wui and co2mpas (v4.1.2), using the windows cmd, and the model graph was working from both cmd and WUI.

ankostis commented 5 years ago

Research

Launching Vagrant: W10-client, installing co2mpas-4.1.1 and launching console.

My purpose is to find who is calling the dot with the CMD.EXE.

I know that co2mpas uses graphviz library, which is a lightweight library calling Graphviz as subprocess.

I visit its sources, and decide that i want to insert a breakpoint() in backend.py:line#157, right before it launches Graphviz's dot.exe.

I need to find it in the local filesystem and edit its sources:

(base) C:\Users\vagrant\Documents>python
Python 3.7.3 | packaged by conda-forge | (default, Jul  1 2019, 22:01:29) [MSC v.1900 64 bit (AMD64)] :: Anaconda, Inc. on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from graphviz import backend
>>> backend.__file__
'C:\\Users\\vagrant\\CO2MPAS\\lib\\site-packages\\graphviz\\backend.py'
>>>

I edit the above file and insert a breakpoint() as planned, launching co2mpas plot command, and use the browser to open some plot, to make it call to Graphviz where i have "trapped" it:

(base) C:\Users\vagrant\Documents>co2mpas plot
 * Serving Flask app "C:\Users\vagrant\Documents\cache_plot" (lazy loading)
 * Environment: production
   WARNING: This is a development server. Do not use it in a production deployment.
   Use a production WSGI server instead.
 * Debug mode: off
2019-10-14 21:55:12,578: INFO:werkzeug: * Running on http://127.0.0.1:5000/ (Press CTRL+C to quit)
2019-10-14 21:55:13,634: INFO:co2mpas:Done! [14.77 sec]
> c:\users\vagrant\co2mpas\lib\site-packages\graphviz\backend.py(160)run()
-> try:

Ok, debugger has landed where we planed. Let's review the variable values:

(Pdb) ?

Documented commands (type help <topic>):
========================================
EOF    c          d        h         list      q        rv       undisplay
a      cl         debug    help      ll        quit     s        unt
alias  clear      disable  ignore    longlist  r        source   until
args   commands   display  interact  n         restart  step     up
b      condition  down     j         next      return   tbreak   w
break  cont       enable   jump      p         retval   u        whatis
bt     continue   exit     l         pp        run      unalias  where

Miscellaneous help topics:
==========================
exec  pdb

(Pdb) list
155             kwargs['stdin'] = subprocess.PIPE
156         if capture_output:
157             kwargs['stdout'] = kwargs['stderr'] = subprocess.PIPE
158
159         breakpoint()
160  ->     try:
161             proc = subprocess.Popen(cmd, startupinfo=get_startupinfo(), **kwargs)
162         except OSError as e:
163             if e.errno == errno.ENOENT:
164                 raise ExecutableNotFound(cmd)
165             else:
(Pdb) print kwargs
{'cwd': '\\\\?\\C:\\Users\\vagrant\\Documents\\cache_plot\\static', 'stdout': -1, 'stderr': -1}

and confirmed that cwd contains a UNC-path that is causing the problem.

But why??

Strangely enough, changing-dir (CD) to a UNC-path is ok:

(Pdb) import os
(Pdb) os.chdir('\\\\?\\C:\\Users\\vagrant\\Documents\\cache_plot\\static')

So here is one prospective fix: change-dir before calling Graphviz.

Let's inspect the command:

(Pdb) p cmd
['dot.bat', '-Tsvg', '-O', 'tmpxgxf9c1m']

Oups, maybe this is the problem, it is not dot.exe but dot.BAT! So maybe if we change it to exe and it works ok, we have found another fix:

First we have to locate the dot.exe:

(base) C:\Users\vagrant\Documents>where dot
C:\Users\vagrant\CO2MPAS\Library\bin\dot.bat

(base) C:\Users\vagrant\Documents>type C:\Users\vagrant\CO2MPAS\Library\bin\dot.bat
@echo off
%~dp0.\graphviz\dot.exe %*

So dot.exe is in ./graphviz from where dot.bat is located: graphviz\\dot.exe

Let's copy that in the clipboard and use it in debugger to change the route of things...

(Pdb) cmd
['dot.bat', '-Tsvg', '-O', 'tmp8rp747ah']
(Pdb) cmd[0] = 'graphviz\\dot.exe'
(Pdb) cmd
['graphviz\\dot.exe', '-Tsvg', '-O', 'tmp8rp747ah']

... and continue

(Pdb) continue
2019-10-14 23:07:51,010: INFO:werkzeug:127.0.0.1 - - [14/Oct/2019 23:07:51] "GET /core.html HTTP/1.1" 200 -

Yeap, it worked! Graph rendered in the browser.

We have found 2 workarounds, but still have not discovered why it runs on some cases? Maybe library has changed to using dot.BAT instead of the dot.EXE?

From is the history of the file we cannot fins the content!

Let's visit the stacktrace to see who is building the command:

(Pdb) up
> c:\users\vagrant\co2mpas\lib\site-packages\graphviz\backend.py(209)render()
-> run(cmd, capture_output=True, cwd=cwd, check=True, quiet=quiet)
(Pdb) l
202         dirname, filename = os.path.split(filepath)
203         cmd, rendered = command(engine, format, filename, renderer, formatter)
204         if dirname:
205             cwd = dirname
206             rendered = os.path.join(dirname, rendered)
207         else:
208             cwd = None
209  ->     run(cmd, capture_output=True, cwd=cwd, check=True, quiet=quiet)
210         return rendered

So it is backend.py:command() deciding, let's vew this in GitHub: https://github.com/xflr6/graphviz/blob/master/graphviz/backend.py#L110-L134

And the same from the local file:

(Pdb) l 110, 136

110     def command(engine, format_, filepath=None, renderer=None, formatter=None):
111         """Return args list for ``subprocess.Popen`` and name of the rendered file."""
...

124
125  ->     if PLATFORM == 'windows':
126             engine = '%s.bat' % engine
127         output_format = [f for f in (format_, renderer, formatter) if f is not None]
128         cmd = [engine, '-T%s' % ':'.join(output_format)]
129         rendered = None
130

...

136         return cmd, rendered
(

Actually there are 2 extra lines (125 & 126) in our local version of the file that append the .bat suffix! Where did those came from?

From the history of the file in GitHb we cannot find that content.

So it must have been added by the conda recipe. Download the package from anaconda-cloud, extract the recipe, and we find this patch:

build:
  number: 0
  noarch: python
  script:
    # Needed due to the Windows batch script wrappers in conda-forge Graphviz.
    # see https://github.com/conda-forge/graphviz-feedstock/blob/bbfe3c7be2a448dd11db165a18c3edfd5ee6a95d/recipe/bld.bat#L27-L32
    # Must patch manually as the source code and patch are CRLF and conda-build
    # tries to convert the patch to LF, which then won't apply.
    - pushd "{{ RECIPE_DIR }}"
    - cp windows-bat.patch "{{ SRC_DIR }}"
    - popd

Conclusions

We unraveled the mystery:

it is conda's Graphviz that calls dot through auto-generated BAT files,
it is the CMD.exe running these BAT files that breaks with schedula's UNC paths for deeply nested modelgraph plots.
there are 2 workarounds described above, both are decent, in the sense that we do not forgo the long-paths:
- 1. either CD to each folder to generate plots 9rather invasive change for schedula), ot
- 1. provide the render command as graphviz\\dot.exe.

ankostis commented 5 years ago

@vinci1it2000 i didn't expect you to know how to navigate conda feedstock, but the debugging of flask + python-graphviz launching Graphviz(the dot.exe command) to discover which where the BAT file intervening, were well within your capabilities AND responsibilities.

But to start doing it, you first had to reproduce the errors. Which you couldn't. Because you do not use the platform-independent tools we have agreed to.

It is not the 1st time this has happened. Remember the con directory you created in you Macr that wreacked havoc on our PCs, hours before the 1st co2mpas workshop? But then at least you had a valid excuse: the AIO did not run in Macintosh. This excuse is not valid anymore - we have Vagrant, it runs everywhere, i use that it on my Linux. You should start using it also. Daily.

spycon69 commented 5 years ago

@ankostis thanks for the well-documented explanation. To my understanding both of the proposed workaround could resolve the issue. But which is the one you propose in regards to stability & effort time ?

stefanocorsi commented 5 years ago

@ankostis great analysis, thanks! Have you already investigated why the conda recipe patches backend.py by inserting the call to the auto-generated batch files?

Do we simply remove this behavior from the recipe or how would you suggest to proceed?

stefanocorsi commented 5 years ago

They say here:

https://github.com/conda-forge/python-graphviz-feedstock/commit/afb92f9cfd51c859aa127176e642514f90087856#diff-e13440d91bcaaab030104d60f0ade38a

# needed due to the Windows batch script wrappers in conda-forge Graphviz

Could we not (I know this is over-simplification) change the recipe for graphviz from this:

    if PLATFORM == 'windows':
        engine = '%s.bat' % engine

to this:

    if PLATFORM == 'windows':
        engine = 'graphviz\%s.exe' % engine

because at the end of the day the only thing that the .bat file does is calling the exe...

stefanocorsi commented 5 years ago

Update: I can confirm the fix suggested by @ankostis works... at least on the vagrant machine. So if we simply could change in the graphviz recipe from :

    if PLATFORM == 'windows':
        engine = '%s.bat' % engine

    if PLATFORM == 'windows':
        engine = 'graphviz\%s.exe' % engine

the plots would display correctly. What do you @ankostis and @vinci1it2000 think?

ankostis commented 5 years ago

@Stefano.

Yes, that's exactly the 2nd workaround, above, served with a monkeypatch. I prefer it, if it doesn't have any other consequences. See my sample code at the bottom.

Research (continued)

Some further links on the conda's decision to use .bat files, which has affected many projects downstream:

ContinuumIO/anaconda-issues#1666: big discussion why Continuum had to do the BAT trick (TL;DR, conda re-packages precompiled Graphviz which contains incompatible QT binaries that can't live in PATH, so they moved all EXEs to the .graphviz/ subfolder)
conda-forge/python-graphviz-feedstock#6: the PR implementing the batches - some good links.
pydot/pydot-ng#52: eloquent ramblings from pydot's maintainer (a downstream project also affected by BATs), where there is this battle-tested code for detecting conda and parsing command-lines.
conda-forge/graphviz-feedstock#34: an issue to "watch", in search for a better alternative to installing conda's Graphviz on Windows- i commented there our problems.

Given all this read, its good to know that the forthcoming Graphvizv-2.40 that would compile cleanly in Windows (CMake γαρ...) and really solve all of our problems (current version is 2.38),

Workaround:

We have to monkey patch graphviz python library as Stefano suggested.

[edit:] It's definitely schedula that must do the monkeypatch i (in 16, but must then wait for a new release, which is not that easy at this moment), so better do it now, in co2mpas.

Workaround for `co2mpas`

def monkeypatch_graphviz-for_conda():
    # Don't patch if not a conda
    is_conda = os.path.exists(os.path.join(sys.prefix, 'conda-meta'))
    if os.name == 'nt' and not os.path.splitext(prg)[1] and is_conda:
        ## Monkeypatch `graphviz/backend.py` as described by Stefano...

Workaround for `schedula`

Posted in vinci1it2000/schedula#16

ankostis commented 5 years ago

I can do a PR for co2mpas.

@vinci1it2000 in which co2mpas-module would you like me to put the monkeypatch code, so as to run only once when co2mpas is imported?

vinci1it2000 commented 5 years ago

@ankostis I disagree with your suggestion of doing the monkey patch in schedula. This problem belongs to jrcstu-forge and conda. Moreover, the patch that you suggested is not applicable when graphviz is installed from pip. This is the reason why I cannot reproduce the error on my window and why I told you several times that the problem is on the exe and not in schedula.

Regarding your comment https://github.com/JRCSTU/co2wui/issues/146#issuecomment-541988574, honestly, I can just say that this is not the way to work. I'm really disappointed for how you behave. Ah just to know, I wrote https://github.com/JRCSTU/co2wui/issues/146#issuecomment-540022890 with the account of @sapofra.

vinci1it2000 commented 5 years ago

@stefanocorsi The easiest solution is to add a recipe in jrcstu-forge for graphviz without the conda patch. This will solve all issues. We have to tackle the source of the issue.

ankostis commented 5 years ago

SUMMARY: Feedstocking our own python-graphvis is technically doable, but it needs more effort & time than monkeypatching the existing one.

Our problem is like this:

schedula is using graphviz lib, a python wrapper around for Graphviz suite.
This lib is called python-graphviz in conda. It is this "feedstock" that needs to to tackle this issue, and the devs seem to admit it when i wrote them.
Until they fix it, we have to monkey-patch it ourselves - much less effort than creating & maintaning our own feedstock.
Monkeypatching should happen as close as the root cause - it is schedula using this package AND UNC-paths, it has to do it.
But we can't want for a new schedula release either, so co2mpas is next on the list.

To give you an example, if schedula were using pydot, another Graphviz all-python wrapper, i would have reported the same issue to them, but still we should be monkeypatching till they fixed it.

ankostis commented 5 years ago

the patch that you suggested is not applicable when graphviz is installed from pip.

I know. The code i gave checks that.

Please reply to my technical question to close this asap.

vinci1it2000 commented 5 years ago

Sorry, but your analysis was wrong. You installed the wrong library (https://github.com/vinci1it2000/schedula/issues/16#issuecomment-542748586).

Please, next time be more accurate on the thing that you do and polite, we lost almost two weeks for a wrong package that you installed.

Please apply the fix suggested in https://github.com/JRCSTU/jrcstu-forge/issues/7.

ankostis commented 5 years ago

You did this mistake 11 days ago, and you continue.

In conda you need BOTH packages, not ONE or the OTHER as you seem to suggest.

python-graphviz (python) and
graphviz the native OS suite.

ankostis commented 5 years ago

Please, once again, don't close issues prematurely.

spycon69 commented 5 years ago

@ankostis @vinci1it2000 github is NOT the place for personal arguments. I am sure that we can discuss in another meeting this and any other pending issues. So PLEASE lets put aside personal arguments and focus on resolution and not argument. Thank you both.

JRCSTU / co2wui

CO2MPAS model graph "Not Found" error #146

Research

Conclusions

Research (continued)

Workaround:

Workaround for `co2mpas`

Workaround for `schedula`

JRCSTU / co2wui

CO2MPAS model graph "Not Found" error #146

Research

Conclusions

Research (continued)

Workaround:

Workaround for co2mpas

Workaround for schedula

Workaround for `co2mpas`

Workaround for `schedula`