-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
script scanner load experiments from directories #229
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@aransfor @theoriginaljuice @gregllong Could you please review this PR? |
@fanmingyu212 I have some time to look at this today. First impressions:
I am sure this is the way to go I am just struggling to get it to run |
@fanmingyu212 I just switched back to master and verified that everything is working. Let me know if you dont see any of this strange hanging behavior and Ill look closer into it |
@aransfor Issue 2, 3: SS should work both from command line and from node. Issue 4: It now auto creates the directory structure in registry, with a key named Please try pulling latest commits to see if they resolve the issues. If SS is still hanging, could you send the full output when you run it in command line, and the value of |
I re-ran the server and it starts but when I run the GUI it gives me the following error Not connected Traceback (most recent call last): |
and then the server hangs |
@fanmingyu212 I got it to work but it seems I cannot select two directories up? if I set the directory key value to Qsim.scripts it breaks but if I set it to Qsim.scripts.experiments it works, can you try different directory levels on your end? |
@aransfor I tried including Qsim.scripts in the directories key. It seems like hanging but it is actually loading slowly (Took ~1 min on our computer till it successfully loaded the experiments). The reason that it loads slowly is that SS attempts to import every python module in the directory and check if the module has a docstring including the script scanner loading information. Importing modules that have many functions that are not enclosed in a class is slow, and there are many such scripts in the Qsim.scripts folder. Therefore, SS just loads very slowly. I think if you have a specific directory for experiments, it is probably good to include only that directory in the |
@aransfor Could you let me know if it works when you set the path to the experiment folder? If it is only a lag issue, can we merge this PR and raise another issue about it? |
@fanmingyu212 gonna try and get to it tonight |
@fanmingyu212 when I have the folder pointed at Qsim.scripts.experiments it loads almost instantly however when I point it one folder higher it hangs at ScriptScanner starting... I let it run for 5 full minutes and still hasnt loaded, but it seems like something is wrong since I have only included twice as many files and it is taking many thousands of times longer to load if it will load. I will leave it on over night and try but in the end we will need a faster solution if this is what is going on. |
@aransfor I think I have figured out why the SS load slowly when the path is set to If you add I tried adding I think the solution is either adding |
@theoriginaljuice can you review? |
@fanmingyu212 do you know if this PR solves this issue: #231 |
@fanmingyu212 I have not forgotten about this its seems to work on my end. I am just very busy right now so be patient I would like to test it further |
@theoriginaljuice can you check that this is backward compatible with your code then we'll go ahead and merge |
@gregllong can you please check backwards compatibility as well? |
@gregllong and @theoriginaljuice , can you both check if experiments load with this code? |
LGTM |
Okay I guess I missed it. The label was never there. Should we have a refresh textlabel for the refresh button? |
@gregllong it had a logo but it only works in linux. Feel free to find a different solution (like putting boring refresh text lol) |
@gregllong I think if you use this branch, the text "refresh" would be displayed. Works on our computers running Win10. |
@theoriginaljuice could you check if your experiments loads with this code? |
Addresses #226 and #228.
List of changes:
QIcon.fromTheme
only works on x11 environments (doesn't work on Windows).allowed_concurrent
is now passed in from initialization. This change is necessary for the changes above and should not influence how experiments are run. Please check if your concurrent experiments work as we do not have any concurrent experiments to test.How to add experiments based on their directory:
["repository1_name.lib.control.experiments", "repository2_name.lib.control.experiments"]
.name
should be changed to the name of your experiment class in this file,load_into_scriptscanner
to whether you want to load the experiment into script scanner, andallow_concurrent
a list of experiments that can run concurrently with this experiment.This docstring should be added as the docstring for the experiment module (at the beginning of .py file), but not the docstring for the experiment class. If you already have a docstring for the experiment module, you can add it to your existing docstring.