CONCORE for Windows enabled #50
Conversation
|
Sure, take your time. Let me know when it is ready for review. |
|
Nice @Rahuljagwani It is similar to what I told in the discussions But I was Also implementing this |
|
@pradeeban will I implement this, as also created this issue |
|
@pradeeban this can be reviewed now. |
|
We will test with main.py and test.py as that is our usual approach. Thanks. |
| # To upload multiple file. For example, /upload/test?apikey=xyz | ||
| @app.route('/upload/<dir>', methods=['POST']) | ||
| def upload(dir): | ||
| apikey = request.args.get('apikey') |
There was a problem hiding this comment.
This is actually intentional. Although not properly implemented yet, the rationale is to have a Kong API gateway (you can see it in the Dockerfile) to sit in front of the FRI.
So, based on an API key, a folder is created. In a server deployment, many users will access FRI with an API key. So, you can have a folder name "test" and I also can have the same folder name. With the API key in place, such parallel server execution (which we do not use much at this point) is allowed.
Pls do not remove this (from here or from test.py). But if you say there are methods that do not have this apikey in place, you should rather add the API key in place for those methods.
That way, for now, we can hard code "xyz" or whatever as an API key and eventually use an actual API key with the Kong - for the server deployment.
There was a problem hiding this comment.
This is actually intentional. Although not properly implemented yet, the rationale is to have a Kong API gateway (you can see it in the Dockerfile) to sit in front of the FRI.
So, based on an API key, a folder is created. In a server deployment, many users will access FRI with an API key. So, you can have a folder name "test" and I also can have the same folder name. With the API key in place, such parallel server execution (which we do not use much at this point) is allowed.
Pls do not remove this (from here or from test.py). But if you say there are methods that do not have this apikey in place, you should rather add the API key in place for those methods.
That way, for now, we can hard code "xyz" or whatever as an API key and eventually use an actual API key with the Kong - for the server deployment.
Understood @pradeeban I will be adding them. The sole purpose to remove them was at some paths only the dir_name was there without API key, but you answered that also. Now I will be adding them performing all tests and revert back to you
There was a problem hiding this comment.
Thanks, yes.
The fix is to either remove them altogether (which you did) or add them uniformly (which is a more future-proof method - because, in case of a missing API Key, we can always have an empty string or a default string. So nothing really breaks that way, whether we have Kong or not).
|
|
||
| # function to check build | ||
| def build(dir, graphml, apikey): | ||
| url = "http://127.0.0.1:5000/build/"+dir+"?"+"fetch="+graphml+"&"+"apikey="+apikey |
There was a problem hiding this comment.
Instead of removing the "&"+"apikey="+apikey, you should actually add this uniformly across all methods and test it.
|
The code looks neat. I just left some comments on removing the "Apikey." I think they should not be removed. Instead, added uniformly across the methods if they were missing in certain places. |
|
@pradeeban |
mkconcore.py
Outdated
| prefixedgenode = "" | ||
| sourcedir = sys.argv[2] | ||
| outdir = sys.argv[3] | ||
| apikeyindex = sourcedir.find('_') |
There was a problem hiding this comment.
My suggestion is to avoid changing anything in this file.
The rationale goes like this:
The API Key implementation is futuristic (not current). It is created when invoked through the front-end. The API Keys are interpreted by Kong and passed along.
So, when a call is initiated with the correct API key, it is mapped with the correct folder.
When the API key is wrong, Kong drops the call. When the API Key is correct (as confirmed by Kong), it accepts the call and sends the call to the FRI backend (together with the API Key), which is used to create the folder ("build"), and invoke the folder ("debug", "run", ...).
When Kong is not present, the call succeeds (with the API Key, regardless of whether the API key is valid or not). This is what happening now, when you pass the hard-coded value "xyz."
So, the proposed changes I have for this PR is:
- Can you make a PR without changes to mkconcore.py? I believe your code should work as-is, with just the changes to main.py and test.py. API Keys are interpreted by Kong, and we shouldn't have to interpret it like this with Windows vs. Linux.
- If someone gives an API key "", the code should still work. But without appending "_"
So an additional check to ensure the input "apikey" value is not empty before appending "_" + apikey.
If it is empty, just do not add anything.
This will still work correctly because:
- when Kong is present and configured to expect API Keys (future status), it will drop the calls without the API Key.
- when kong is not present (current status), it will accept the calls with or without the API Key.
There was a problem hiding this comment.
My suggestion is to avoid changing anything in this file.
The rationale goes like this: The API Key implementation is futuristic (not current). It is created when invoked through the front-end. The API Keys are interpreted by Kong and passed along.
So, when a call is initiated with the correct API key, it is mapped with the correct folder. When the API key is wrong, Kong drops the call. When the API Key is correct (as confirmed by Kong), it accepts the call and sends the call to the FRI backend (together with the API Key), which is used to create the folder ("build"), and invoke the folder ("debug", "run", ...).
When Kong is not present, the call succeeds (with the API Key, regardless of whether the API key is valid or not). This is what happening now, when you pass the hard-coded value "xyz."
So, the proposed changes I have for this PR is:
- Can you make a PR without changes to mkconcore.py? I believe your code should work as-is, with just the changes to main.py and test.py. API Keys are interpreted by Kong, and we shouldn't have to interpret it like this with Windows vs. Linux.
- If someone gives an API key "", the code should still work. But without appending "_"
So an additional check to ensure the input "apikey" value is not empty before appending "_" + apikey. If it is empty, just do not add anything.
This will still work correctly because:
- when Kong is present and configured to expect API Keys (future status), it will drop the calls without the API Key.
- when kong is not present (current status), it will accept the calls with or without the API Key.
Not For Kong
I understand what is being conveyed, but if no changes in mkconcore.py are entertained then it will not be possible to create out_directory of name 'graphmlfilename_apikey' in case user provides api key. If this idea of making folder by the name format 'graphmlfilename_apikey' has to be dropped, then there is no use of passing apikeys to functions (debug, run, stop, clear etc.). Build certainly needs api key if passed which is the neutral case.
So the main concern is whether or not api keys are passed in debug, run, clear, stop, download etc., anyway they will perform their respective functions. The only use I find is that if the out_dir with name of graphml file is attached with apikey (eg. sample_xyz which can only be created by making changes in mkconcore.py) and we need to access it, then passing apikey is beneficial. (For Linux, Windows etc.)
For Kong
When it comes to Kong, apikeys must be passed. Therefore all changes conveyed by you are perfect for now, and they will become more clear after Kong configuration will be completed. Therefore, I will be making changes as proposed above.
|
|
||
| def getFilesList(dir, sub_dir = ""): | ||
| url = "http://127.0.0.1:5000/getFilesList/" + dir + "?"+"fetch="+sub_dir | ||
| def getFilesList(apikey, dir, sub_dir = ""): |
There was a problem hiding this comment.
In your other methods, apikey was the last argument. Please keep the consistency and leave it as the last argument.
There was a problem hiding this comment.
In your other methods, apikey was the last argument. Please keep the consistency and leave it as the last argument.
So one problem is there in doing so as non-default arguments cannot be followed by default arguments. There are few fixes of this problem to maintain consistency:
-
Make apikey a default argument : There is a small issue with this as if someone calls getFilesList from test.py and not want to pass sub_dir but only apikey. In that case apikey will go into sub_dir. But I think no one will majorly call function as test.py is for testing purpose.
-
By function overloading : 4 (3 new + existing 1) different functions have to be created .
-
Make 'apikey' argument as first argument in all functions.
I will implement one of them after getting conformation.
Thanks
There was a problem hiding this comment.
Yes, as long as you keep the changes to FRI and ensure your follow-up changes do not break any of the progress that you have made in this integration, it is cool. We can always go incrementally.
fri/server/main.py
Outdated
| dir = secure_filename(dir) | ||
| dir_path = os.path.abspath(os.path.join(concore_path, dir)) | ||
| proc = call(["./debug"], cwd=dir_path) | ||
| apikey = request.args.get('apikey') |
There was a problem hiding this comment.
Please make sure the "apikey" is an optional argument.
In the presence of Kong configured to expect an API Key, it will drop the calls without a valid API key. So it will drop calls with apikey params that are empty or invalid (such as "xyz").
But when Kong is not configured (current status), we should support calls for both apikey that is not present or random alphanumeric characters (such as "xyz123").
Proposed Action: No additional checks are needed here at this point. But just make it an optional field.
There was a problem hiding this comment.
Please make sure the "apikey" is an optional argument.
In the presence of Kong configured to expect an API Key, it will drop the calls without a valid API key. So it will drop calls with apikey params that are empty or invalid (such as "xyz").
But when Kong is not configured (current status), we should support calls for both apikey that is not present or random alphanumeric characters (such as "xyz123").
Proposed Action: No additional checks are needed here at this point. But just make it an optional field.
Understood
|
Thanks, @Rahuljagwani. I have added a few points. But the most important of the comments is any changes to this should be limited to only FRI (both main.py and test.py.) Please do not include any changes to mkconcre.py in this PR. Please let me know if I am missing something or if something is unclear. Even for the GSoC, I mostly see the changes in concore-editor and FRI only. FRI functions as the REST server backend for the concore-editor, creating a bridge between concore and concore-editor. |
@pradeeban First of all, thanks for reviewing my code . Changes proposed for this PR
Therefore, for now all functions inside main.py will take apikey argument but not do anything significantly with it, except (build, uploadfiles) I will be making above changes. |
|
@pradeeban |
mkconcore.py
Outdated
| prefixedgenode = "" | ||
| sourcedir = sys.argv[2] | ||
| outdir = sys.argv[3] | ||
|
|
There was a problem hiding this comment.
Can you eliminate this file from the PR? I think you entered a new line here, so it is included.
The reasoning is this file is one of the most used files and often with local changes. Any change made to this file is trouble. :)
There was a problem hiding this comment.
Can you eliminate this file from the PR? I think you entered a new line here, so it is included.
The reasoning is this file is one of the most used files and often with local changes. Any change made to this file is trouble. :)
Sure I will do that right now.
There was a problem hiding this comment.
Can you eliminate this file from the PR? I think you entered a new line here, so it is included.
The reasoning is this file is one of the most used files and often with local changes. Any change made to this file is trouble. :)Sure I will do that right now.
@pradeeban mkconcore.py is now same as original one.
And also,
Huge Congratulations!!!!! on getting this organisation selected for GSoC 2023.
There was a problem hiding this comment.
Can you eliminate this file from the PR? I think you entered a new line here, so it is included.
The reasoning is this file is one of the most used files and often with local changes. Any change made to this file is trouble. :)Sure I will do that right now.
@pradeeban mkconcore.py is now same as original one. And also, Huge Congratulations!!!!! on getting this organisation selected for GSoC 2023.
As I Know the selected organizations are not yet announced? Yeah but you are right this will be one of that
There was a problem hiding this comment.
Can you eliminate this file from the PR? I think you entered a new line here, so it is included.
The reasoning is this file is one of the most used files and often with local changes. Any change made to this file is trouble. :)Sure I will do that right now.
@pradeeban mkconcore.py is now same as original one. And also, Huge Congratulations!!!!! on getting this organisation selected for GSoC 2023.
As I Know the selected organizations are not yet announced? Yeah but you are right this will be one of that
https://summerofcode.withgoogle.com/programs/2023/organizations check!!
|
Thanks. The PR looks good. We can handle the refactoring and cosmetic issues later separately in a new PR. Only one comment before I merge this PR.
One issue (a bug from the early days of FRI) I see is, But again, that is a separate issue - irrelevant to your PR on making this work on Windows. |
@pradeeban |
|
Aha, right. Yes, that is a separate PR. We can review that one separately and in more detail. (Since you already have that fix handy, you can send a PR right away after I merge this). I can merge this one. I still notice mkconcore.py in this PR due to one line being changed (check the very last line). Can you make sure not to change that file in the PR? |
|
If it is easier, you can just create a new PR from a separate branch. (Just making sure only FRI is changed in this PR). In fact, this PR is more like "concore FRI for Windows enabled." (FRI is the File Receiver Interface, which provides a RESTful interface to otherwise just a set of Python programs of concore). |
Sure I will be doing this. |
|
Closing this as continued on #51 |
3 participants
@pradeeban On Linux and Mac OS, can be tested.
But don't merge it as some testing on windows is remaining.
I will let you know once ready.