Make Rosie Work under packaging (branched from #2360)

[wxtim]

PR #2360 deactivated tests for rosie and rosa in the packaged version of rose. This ticket is to re-activate them:

• Reactivate all skipped tests and check that these work.
  • Rosa
    • rosa svn-pre-commit
    • rosa svn-post-commit
    • rosa db-create
  • rosie
    • checkout
    • create
    • delete
    • disco
    • graph
    • hello
    • id
    • lookup
    • ls
    • .db

[oliver-sanders]

May want to rebase - squash :)

[oliver-sanders]

Note that at present pip installing Rose doesn't quite work as due to an incompatibility with 8.0a0 which is all we have on pipy right now. I've tested against master using the following diff and all is fine:

diff --git a/setup.py b/setup.py
index 8303bd3c3..062720788 100644
--- a/setup.py
+++ b/setup.py
@@ -50,7 +50,10 @@ with open("README.md", "r") as fh:
 
 INSTALL_REQUIRES = [
     "jinja2>=2.10.1, <2.11.0",
-    "cylc-flow==8.0a0",
+    (
+        'cylc-flow @ https://github.com/cylc/cylc-flow'
+        '/tarball/master#egg=cylc-8.0a1.dev'
+    ),
     "aiofiles",
     "tornado",
     "sqlalchemy",

[oliver-sanders]

Technically sound, tested as working.

Some documentation issues resulting from an over-zealous search and replace:

[sadielbartholomew]

(Unassigning myself for review, as discussed with Tim offline. I'm not the best placed to review this one, having little experience with packaging.)

[oliver-sanders]

OK, I think we are good to go here, you've managed to do that thing with your commits where they are all duplicated for some reason. Can you squash them down? And how the heck does this happen anyway!

[wxtim]

OK, I think we are good to go here, you've managed to do that thing with your commits where they are all duplicated for some reason. Can you squash them down? And how the heck does this happen anyway!

I think this was caused by me working on a branch of a branch whilst the first branch was under review and the rebasing or merging badly after making changes to the first branch.

I can't squash them easily. I took a diff and created something new that should be tidier. Detail has been lost though.

[oliver-sanders]

Tentative approval! I think this is OK, installed locally pip install -e ..

[kinow]

Installed via pip install -e . with no issues.

(venv) kinow@kinow-VirtualBox:~/Development/python/workspace/rose$ python -c 'from metomi.rose import __version__;print(__version__)'
2.0.0alpha3

pip show -f metomi-rose is behaving a bit weird, but in the issues I found related to the message "Cannot locate installed-files.txt" the issue was actually elsewhere (e.g. in pip or wheels).

And after installing with pip install . (not editable) pip show -f metomi-rose works. Looking at the files generated, METADATA is missing platform, but I don't think that's a blocker. I don't know rose well enough, but I think I spotted something that could prevent rosie from working, so will add comments for @wxtim or somebody else to take a look.

But great work! You did it really quick, something that took me over a month to get it right-ish for Cylc Flow haha. 👏 And packaging in Python is probably one of the hardest, when comparing with other programming languages like PHP, JavaScript, and Java.

[kinow]

@wxtim I have finished adding a bunch of comments, then re-read the ticket description, but from this part "this ticket is to re-activate them", I reckon you've accomplished what was desired here.

Feel free to resolve my comments and merge if you'd like. These comments can all be tackled in another ticket later, as I think this one was just to re-enable tests.

[kinow]

This is raising an issue in my virtual environment, due to pygraphviz not being installed after pip install .. I don't see any graphviz related dependencies in setup.py... is this file used by rose or rosie? If so, we may need to include the dependencies in setup.py.

[wxtim]

Yes. Thanks!

This is nasty: 😞 - I'm afraid there is more work to be done on removing pygraphviz from the dependency tree in both rose and rosie. I failed to notice this because of the skips in the tests.

Pygraphviz can be installed via conda, but there is no surety of installing it correctly using pip, and therefore, since we are planning to remove it anyway it should be removed. It looks fairly involved, however, since it needs to change rosie graph and rose graph significantly so with the permission of @kinow @oliver-sanders I'm going to suggest that it should be a separate issue.

[oliver-sanders]

Yep, this dependency has been left floating about, it will be removed in due course, I think we will copy the solution from the new Cylc Graph.

[oliver-sanders]

For now either delete rosie graph or add pygraphviz to the dependencies (note it's a bit of a pain to install).

[wxtim]

@kinow I'm afraid I'm just going to leave this for this PR and create a new issue to deal with removing this. There's no point getting this test working as the item tested will be removed and re-written in the new GUI. I do not want to include pygraphviz in the package dependencies since the need for it will be removed.

issue #2369 created

[kinow]

Really didn't want to block the PR. If you prefer to move some of the work here for later please feel free. I just got confused because I read the title and went straight to review it without reading the issue description, sorry!

One thing that I saved for later but that's bugging me is an alert on my IDE, that I tested later and indeed didn't work... again, not blocker, but if you are looking at the code again, perhaps you could try and let me know if this works for you (I think your environment is slightly different with miniconda, conda, etc... so it could be my environment).

(venv) kinow@ranma:~/Development/python/workspace/rose$ python
Python 3.7.3 (default, Mar 27 2019, 22:11:17) 
[GCC 7.3.0] :: Anaconda, Inc. on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from metomi.rose.macro import get_macros_for_config
>>> get_macros_for_config(config=None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/kinow/Development/python/workspace/rose/metomi/rose/macro.py", line 802, in get_macros_for_config
    config = metomi.rose.config.ConfigNode()
UnboundLocalError: local variable 'metomi' referenced before assignment
>>> 

[wxtim]

No @kinow , I think I should bite the bullet and do it. So nearly there...

[kinow]

No @kinow , I think I should bite the bullet and do it. So nearly there...

In that case happy to try to help testing. Let me know if you need anything 👍

[wxtim]

One thing that I saved for later but that's bugging me is an alert on my IDE, that I tested later and indeed didn't work... again, not blocker, but if you are looking at the code again, perhaps you could try and let me know if this works for you (I think your environment is slightly different with miniconda, conda, etc... so it could be my environment).

Had the same issue - have had a go at fixing it.

[wxtim]

Annoyingly this is now failing because of the issue described in #3216. 🤦‍♂️

[wxtim]

from metomi.rose.macro import get_macros_for_config
get_macros_for_config(config=None)

@kinow - Think I've fixed this one!