rosie & rosa: migrate to Python 3(.6-.7) & Tornado

[sadielbartholomew]

Address point 1 of #2271, namely for the Rosie Discovery service, with minimal changes [:star:]:

• migrate the web server from CherryPy to Tornado;
• port necessary components (only) from Python from 2 to 3, not preserving backward compatibility, as an intermediate step, consisting of:

  • modules i.e. rosie.ws & its chain of undetachable module dependency imports:

    rose.	rosie.	rosa.
    config date host_select loc_handlers.svn reporter metadata_graph [close Convert Rose Metadata Graph to Python 3 #2301]	ws suite_id lookup ls graph	db_create svn_post_commit svn_pre_commit (not essential but fixed as a side effect)

  • tests: relating to the Rosie Disco server & client:

    key: ✔️ = passes locally.

    t/rose...	t/rosie...	t/rosa...
    	.db ✔️ -disco ✔️ -hello ✔️ -graph ✔️ -lookup ✔️ -ls ✔️	-db-create ✔️ -svn-post-commit ✔️ -svn-pre-commit ✔️

Also make some isolated trivial changes to clean up & fix minor issues from #2286.

⭐ Context on changes that may seem non-minimal without explanation:

I added some new classes, but these do not change the functionality. They were created to preserve the current logic but in a way that is compatible with, & organised under, the Tornado framework:

• RosieDiscoServiceApplication organises the collection of request handlers forming the web application. A Tornado application does not have to be a class, but the amount of logic for defining this application merits one to keep everything clean. Logically, the old RosieDiscoServiceRoot -> RosieDiscoServiceApplication + new RosieDiscoServiceRoot.

• QueryHandler & SearchHandler were created as subclasses to RosieDiscoService because they are distinct API points & the most natural & "Tornado-esque" way to adjust the data retrieval is to set up a base class & subclass it to override the HTTP GET. Logically, new QueryHandler = old RosieDiscoService.query() & new SearchHandler = old RosieDiscoService.search(). I then moved the module-level function _query_parse_string() into QueryHandler as a static method, since it concerned only that class, to tidy up.

• In order to keep things as simple as possible, I had to change the CLI a little, since Tornado has a command-line parsing module where options are much simpler to set-up than arguments (so start -> --start etc.). See the re-documented CLI via rosie disco --help / the docstring to bin/rosie-disco for full detail.

• A few tweaks, unavoidable without adding complexity to ws.py, were made to the templates, as they were doing the wrong thing otherwise (looking for a favicon .ico file that did not exist, for example), for reasons I am still unsure of.

TO DO list:

• Get the Rosie Discovery service to run via Python 3.
• Tidy up the Python 2 to 3 port to remove any unnecessary 2to3 tool changes.
• Get a Tornado server to start according to the same rosie disco CLI as before...
• & ditto to stop (cleanly) [update: this was by far the most difficult part of this PR, taking an equivalent proportion of the time!].
• Serve the basic service API points in the absence of data.
• Serve the service search & query API points, to return both:
  • JSON data structures;
  • web pages with tables populated with the results.
• Ensure all possible/documented [update: not all from docs were there originally; supporting only current/original API to minimise changes] query arguments are functional & not persistent [update: this is an issue in the current (master/CherryPy-variant) behaviour, hence leave as-is for this PR & register for fixing later].
• Manual testing for functionality & correct outputs from REST inputs, as above.
• Migrate automated testing.

[sadielbartholomew]

Migrate automated testing: * awaiting discussion before starting.

... Naturally, the test battery & Travis CI will now fail not only on tests relating to rosie disco, but various others given the porting without back compat to Python 3 of only a small subset of Rose & Rosie modules. Though Codacy raised a few valid points which I will address along with the below.

@matthewrmshin I've tried to convey important context in my opening comment. I will fix the JSON ASAP [Fixed in the latest commit!]. At your convenience we should discuss strategy for the testing aspect.

[oliver-sanders]

Nearly 100 lines shorter and that's including additional comments, this makes me happy. Still a WIP but some [perhaps irrelevant] pre-review comments:

[sadielbartholomew]

Update: I made some changes in response to the initial feedback, but I still need to make some amendments to ensure all arguments are processed & do not persist across request instances due to improper class use. The 'TO DO' list has been updated to indicate progress thus far.

[wxtim]

I have had a play - the functionality seems unchanged between the oper version (2019.01.00) and the test version - the only slight change I noticed was that search terms are replaced in the search box with themselves wrapped in quotes and square brackets - i.e. Tim, becomes ['Tim']

[sadielbartholomew]

Thanks for the review feedback, @wxtim, it will be useful going forward. I'm glad the utility seems to be doing as it should except in a few cases in your environment too.

Just to add a reply RE your summary comment, which I can't reply to directly:

only slight change I noticed was that search terms are replaced in the search box with themselves wrapped in quotes and square brackets - i.e. Tim, becomes ['Tim']

Good spotting, I'll add that to the TO DO list for fixes to address before the PR transcends WIP status.

[sadielbartholomew]

@matthewrmshin, @wxtim, @oliver-sanders: FYI, the tests are not yet sorted as I had hoped from yesterday so this is still a WIP sadly, but I will be coming in tomorrow to hopefully finalise it but at the least get it to an agreed stage where you all know how to go forward with it while I am on leave until 12th so I do not delay anything. In the meantime I will be watching this PR and can answer any questions, too.

[sadielbartholomew]

@matthewrmshin, @wxtim, @oliver-sanders:

but I will be coming in tomorrow

Ah, scratch that, I now can't make it as I still have some urgent tasks to do at home that I need to do before a flight this evening. Regardless, I will be keeping an eye on this PR for comments and will likely spend an hour or two on this while on leave in which case I'll comment with any progress.

[matthewrmshin]

Change looks OK by 👀 with 👓. On testing using the ad-hoc server, there seems to be a complete lack of server log? Can this be added? E.g. an access log to find out what requests were received and their return statuses, etc.

[sadielbartholomew]

Can this be added? E.g. an access log to find out what requests were received and their return statuses, etc.

Sure, @matthewrmshin, that would be very useful & I will get it implemented.

[sadielbartholomew]

Sure, @matthewrmshin, that would be very useful & I will get it implemented.

Sorry for the delay, I was inundated with user support over Easter being the only person not on leave & I put this on hold accordingly, given I also knew it would be a while before reviewers returned from holiday.

I have set-up three informational logs, corresponding to Tornado's built-in logging streams (with custom server start & stop messages added for full transparency), to complement the server status log required to parse for cleanly stopping the server, as was already implemented.

[matthewrmshin]

Does Codacy have a point about overriding the get methods with different signatures?

[sadielbartholomew]

Does Codacy have a point about overriding the get methods with different signatures?

No, as far as I understand it; the method that is being overridden has the signature:

rose/lib/python/rosie/ws.py

Line 198 in 2ba14b2

def get(self, *args):

so it should accept any number of positional arguments, required since the sub-classes overriding that method ideally can have different amounts to accommodate their context. I have also manually tested the application quite thoroughly now & have not seen any warnings or errors related to incompatible interfaces for Handler methods.

[sadielbartholomew]

For information, I have just made one final (bar any new review feedback) commit to fix the logging of the server stopping, as I have just noticed that with the recently implemented logging code a line to that effect was not being written out due to the blocking nature of the Python logging utility.

[matthewrmshin]

I think Codacy/PyLint is telling us that if you override a method with signature def get(self, *args):, the sub-class's method should also have the signature def get(self, *args):. I suppose, if the methods in the sub-classes have different contexts, you should:

• Either: stick with the same signature. Raise ValueError if args is expected to be of certain length.
• Or: add some # pylint: disable=... to tell PyLint to shut up.

[wxtim]

I'm still happy with my approval - but have come up with 2 more questions:

1. If I search for something, click on the link top-left and return to the home screen then return to the mi... page the search is retained. Deliberate? Desirable?
2. There's no terminal output when I start the server. I think it'd be nice to give me a URL?

[oliver-sanders]

Have been through and marked a few review comments as resolved.

[sadielbartholomew]

Thanks for elaborating @matthewrmshin. In my next commit I will implement one of your suggestions (ideally the first, but it depends if I can do it in practicable time without making the code too artificial & harder to follow).

In response to your comments @wxtim:

If I search for something, click on the link top-left and return to the home screen then return to the mi... page the search is retained. Deliberate? Desirable?

Desirable, no. It's hard to answer if it is 'deliberate'; I am aware of it but I am not planning to amend it in this PR, as covered this in my comment #2288 (comment) (see the bullet point on 'Persistent query parameters'). The original brief for this PR was to do an absolutely bare-bones CherryPy to Tornado conversion, making no change to behaviour, & the persistence of the URL parameters and their corresponding rendered pages was a problem that was there originally. It is not trivial to fix from some exploratory work I did early on.

There's no terminal output when I start the server. I think it'd be nice to give me a URL?

There should be? I've put a print statement which should show when you start the server (logically, & it shows for me):

rose/lib/python/rosie/ws.py

Lines 465 to 468 in 46abd56

	# Call print before IOLoop start() else it prints only on loop stop.
	print("Started" + user_msg_end)
	IOLoop.current().start()

Though adding a URL is a good idea, thanks. I'll add that in within my next commit. Saying that, can I ask from now on that you are aware that while there is a lot of improvement we can make to this utility, that was not the goal of this PR, so for improvements such as these, please just add them to the list of follow-on work here: #2288 (comment), which will become an Issue once this is merged.

[matthewrmshin]

Codacy/PyLint has a final complaint, which I believe can be eliminated.
Travis CI unhappy.

[sadielbartholomew]

Codacy/PyLint has a final complaint, which I believe can be eliminated.
Travis CI unhappy.

Sorry, I pushed up my branch before I was finished. The above issues should now be resolved. Awaiting test results here to see. Travis CI & Codacy defeated!

[matthewrmshin]

@oliver-sanders @wxtim please do a final sanity check.

[oliver-sanders]

Code all reviewed, Rosie Disco tested as working with two small niggles:

1. rosie disco doesn't seem to print the status of a running server as documented.

• I guess this should just print any status files in the terminal so should be pretty quick to fix.
• Otherwise we could temporally remove or undocument it?

2. If the server crashes or is ctrl-c killed then rosie disco stop results in traceback (as the log files get left behind so it tries to kill a process which isn't there).

• A quick try/except would probably suffice.
• Bonus points for tidying up the redundant status file but that isn't necessary.

[sadielbartholomew]

Thanks, @oliver-sanders, great reviewing!

rosie disco doesn't seem to print the status of a running server as documented.

Ah, well spotted. I've just fixed that in the latest commit.

If the server crashes or is ctrl-c killed then rosie disco stop results in traceback (as the log files get left behind so it tries to kill a process which isn't there).

Good spot again. In the latest commit I've also ensured that the traceback is not longer produced in this case.

Bonus points for tidying up the redundant status file but that isn't necessary.

That will be left as follow-on work, as per bullet point 1 of #2288 (comment).