Mr tool refactor server side Fixes #71

[McCrea]

I think the main issue with adding tools still stands - one shouldn't need to define a new function in order to add a tool since the logic for each tool is very similar. It would be preferable to have a single function (with a single URL) which took the type of gating (e.g. square, oval, poly) and gating parameters (e.g. coordinates, reverse gate etc.) and sent the gating in motion. That way there would be a simple and (more importantly) generic way of adding new tools. It could be that the single function ultimately just checked the type of gating and passed the arguments on to different functions (perhaps one for each type of gate, as happens currently) but having a different URL and view for each gate type seems to add a lot of complexity to adding tools, and won't scale well if there's ever more than a handful of tools. Does anyone else have views on this, or is it just me?

[notchris1]

i think i agree, it doesn't seem like it would be much trouble to make it so much more general and extensible.

[hazeld]

Yeah I agree, seen as all three basic gates need the same parameters it would make sense to just have one function for this and just give it the type of gate as well.

[mrudelle]

I disagree about the definition of a new function for a new tool. This is not just about gating tools, any kind of tool could be implemented in the future here . so just throwing the parameters to the r program is not the best solution in my opinion, Ok these 3 can be simplified further but we would loose some processing (like swapping corner if needed) that will have to be done on clientside. May I remember that a wrong parameters make the instance crash so this level allows to filter the request (that could be absolutely anything as coming from internet.

[McCrea]

As I pointed out earlier, the single function could essentially be a big block of 'elif's in places, calling other methods for more complex tools. If there are three tools which can be simplified, strikes me it's likely that there will be more which could gain from a generic 'gating' handler. I can't see that you'll gain any extra reliability/sanitisation from having separate URLs/views for each tool - the only difference is that a generic handler makes adding new tools considerably easier. Incorrect parameters shouldn't make the instance crash, but whether they were dispatched from a generic handler or a special case handler won't make a difference. Also, using a generic handler could in fact help with sanitising requests as it reduces the likelihood that sanitisation will be forgotten when adding new tools and allows a certain level of sanitisation to be applied consistently across all gating requests.

If you don't want to make the changes yourself, feel free to unassign yourself from the issue and ask for someone else to look at it, but so far it seems most of the group support making the handler generic, so I feel it should be done at some point.

[mrudelle]

Could you be more precise on what you want exactly?

Apart of that, it might be from the way we learn in my university but a bloc of elif sounds really bad to me.

The way I see it is that here you can create any type of tool: first you create your tool on client side, the tool will then trigger a request like tool//params then you catch that on the client side by adding a small method that takes this parameters as an array and return a simple dictionary where you can fill any information that can be useful to the client side. As said earlier this implementation is here for the sake of versatility. for instance you can easily make a tool to delete, remove or rename a file without having to touch the urls.py or views.py

If you want to narrow the scope of this functionality to gating tools only, there is no problem, I can do it. But we would loose simplification when adding general purpose tool.

What I can do on the other hand is to create a tool named gating that will have in it's parameters the gate name. And then implement this tool with a elif or just throw all the parameters (gatename included) to the queue and the r script would handle sorting and filtering the request.

[McCrea]

You're quite right that massive blocks of elifs aren't exactly ideal, however, currently (and for the foreseeable future) we're not going to have enough tools to make that sort of code a real problem. An alternative to a elifs is a dictionary which maps tool names to methods, or a list of method and tool name pairs with an iterator. The list approach is unlikely to ever actually be faster than elifs. The dictionary could potentially be faster than a bad case using elifs, but you'd need a lot of elifs before it was slower than building up a dictionary and then accessing it. The dictionary approach probably isn't a bad one since a clean mapping of names to methods would probably be quite good. And so long as the entire parameter string (including tool name) is passed to methods, several (similar) tool names could map to a single method if required.

There's absolutely no reason why using a single gating handler should make it less versatile than having several. What I have in mind is that you would have a single gating URL, with a single view. When the client side requests gating, it constructs a string to go into GET (or POST, though I feel GET would probably suffice for now). That string is read out by the view on the server side. The first part of the string would be the name of the tool the user had selected, and this would be used by the gating view to determine which method to use to dispatch gating to Compute Engine and return a response. (JSON would probably be preferable to a string, but if I recall correctly it's currently a string and might save time to keep it that way.)

You mentioned being able to delete/rename files without touching urls.py, but, using the current gating approach, I'm not sure how that would be possible. Unless, of course, you were to use the generic /app/data/edit/files/ handler which I wrote, to be generic and to take the action requested as part of the JSON request payload.

[mrudelle]

What you describe is more or less the way things are done now with what I implemented. The response is actually already a JSON.

What I meant by "being able to delete/rename files without touching urls.py" is that to implement such a tool you only have to create an entry in AVAILABLE_TOOLS with the name "data-rename-file" for instance, make it point to a function that do the operation and return a dictionary that will be sent back as JSON to the client-side (with status and redirection for instance).

If I understand correctly what new you would like to be implemented is that the gating tool was actually a single tool (with the url "tool/gating/,"), right? but after you speak about dispatching the qerries. I cannot get exactly where you prefer this dispatching to be done? For now it's done by the dictionary AVAILABLE_TOOLS wich dispatch it in several methods in tools.py. Are you thinking about doing the dispatching from a single method in tools.py while sending the request to the queue?

[McCrea]

The part I was thinking could do with being JSON is the gating request sent from client to server when a user clicks the 'create gating button'. Currently it seems to be purely GET in the form '/app/gating//? Ultimately I think it would be nice if that was a request sent to a URL e.g. (as you suggested) 'tool/gating/' with a request containing a JSON object such as

{ 'toolName' : '<tool name'>,
  'params'    : [.<param>,'*] }

However, using JSON may complicate matters further.

My main aim here is that there be a single gating URL so that when you add a gate you don't have to add a new URL and a new view, just modify the single gating view. That single gating view would likely just look at the tool requested and then call a function for that particular tool with the parameters as arguments. So you might have a URL like '/app/gating/?tool=&' which would pass and into a gating view such as (please excuse any bad syntax - I'm not able to run anything I'm writing at the moment)

tool_mapping = {
'rectangle_gate' : simple_gate,
'oval_gate'  : simple_gate,
'some_complicated_tool : complicated_tool }
## Where 'simple gate' and 'complicated_tool' have a signature like 'def name(tool_name, params):' and are responsible for actually starting the gating process

gating(tool_name, params):
  if tool_name in tool_mapping:
    return make_gating_response( tool_mapping[tool_name](tool_name, params) )
  else:
    return make_gating_response('tool does not exist')

If anyone else has a view on this, it would be good to know.

[mrudelle]

Concerning the implementation of POST instead of GET: My implementation ease largely the refactoring as only the method views.py@tool(request, name, params) would change (and not the tools themselves)

Concerning the rest. I think I get what you suggest. I just want to be sure you (a feedback from many people is better) prefer to dispatch the different tools in a elif bloc rather than in a dictionary? Because I don't see the benefits in term of ease of implementing new tools.

[McCrea]

I'm not concerned about changing GET to POST - I think that's really a minor detail which we don't have time for.

As for dispatching the different tools, I don't mind if it's an elif or a dictionary - whichever works best/looks neatest. That's really just a minor detail - what I'm interested in is making sure it's easy to add new tools in future in a generic way. It's only the difference between adding

    elif (tool_name == 'some_tool'):
      return some_tool(tool_name, params)

and adding

    'some_tool' : some_tool,

As this pull request stands, I think it resolves the issue. As for replacing the dictionary within tools.py with an elif, I'm not sure it was necessary but that's up to you. It's selecting a tool upon request which I was really concerned by and that has been resolved - I really didn't have a problem with each of the current tools having their own dictionary entries. (Using a 'simple gating' tool for all the current gates was really just an example, but if that works well then it's okay.)

I still think there's an outstanding issue or two with these commits, though (see my line comments). Mainly that tools.py really ought to be renamed/moved (I imagine a new directory would be the best place - then very complex tools can have their own files under that directory).

[mrudelle]

So wtf are you blabbering here? "the main issue with adding tools still stands", I don't get the point of what you said at the end...

I would like you to answer clearly to my questions:

• Did the last commit solved the problem of "the main issue with adding tools still stands"?
• And if not, what is it that is wrong? I feel like you beat around the bush here. Because for me, my implementation makes it "easy to add new tools in future in a generic way.". And if say me clearly what is not generic I would love to make the change.

Concerning the logging, I will commit that in a second, idem for the json variable. And concerning the renaming, moving, I propose to put this file in API/appTools.py, any better suggestion?

[McCrea]

I didn't say that the issue still stood with the latest version (at least not on any recent version of this pull request) - I said that this pull request now resolves the original issue but there's a couple of minor issues - namely the filename and the variable named 'json' which, as I understand it, are being fixed.

I would be happy to merge this pull request now, as long as the changes you mentioned you were about to commit have been applied.

I have a feeling that we might have been talking about different parts of the code here - my only concern is essentially that there is a single gating URL, which has been done. How the gating tools are mapped within that function is up to you - I've suggested the odd thing, but my suggestions are no more than suggestions. I apologise if I've been unclear but I've really just been trying to explain my previous points and my concerns, which I feel were shared by the rest of the group

[mrudelle]

Great. The changes are now done and everything is ready to merge.