Support for crYOLO 1.3.3 and big refactor

[delarosatrevin]

There some serious issues:

• Temporary working folder did not reflect the batch group and the work could be overwritten or just duplicated when working with threads.
• Some bugs when iterating by coordinates, but not sorting by micId
• Several performance issues where some computation were repeated unecesary
• Several blocks of unused code that was copied remained there
• A lot of our conventions were violated

New things with cyOLO 1.3.3:

• Filter of the micrographs can be done on the fly
• The training can be done to "fine-tune" an existing model

Updated the README file, there were some outdated variable names.

[azazellochg]

@delarosatrevin is cleaning the Augean stables xD

[pconesa]

@delarosatrevin , we are not able to run the test successfully. I guess you run it and you manage to run the test successfully.

In that case, we are struggling with the environment:

At least in @the-best-elephant conda installation.....conda has to initialize the "SHELL", so any command run from scipion has to be run, triggering the corresponding, basrhrc (tcshrc,....) which now we are forcing bash (but this is secondary). The code is running plain commands like "conda activate cryolo; ..." This does not trigger the corresponding bashrc execution and therefore conda (although is in the path) complains and suggest to do a conda init (which is already done and works fine from a plain terminal. One this we manage to do is, when checking the version, do "bash -ic ...." which works. But can not see a way to pass that -i to the runJob.

So, I guess we should first understand why this differences and how is it working in your case but not in @the-best-elephant machine.

[delarosatrevin]

@pconesa In my activation command, I first "activate" conda...but I guess this can be different from one pc to another, depending how conda is setup. Anyway, in my case I have:

export CRYOLO_ENV_ACTIVATION='. /home/josem/installs/anaconda2/etc/profile.d/conda.sh; conda activate cryolo-1.3.3'

[pconesa]

That might explain it, you do not have it in the config?

[pconesa]

@delarosatrevin we are going to make changes to, at least, allow to set the variable in the config file, now, scipion prepends "SCIPION_HOME" and it shouldn't be touched. Also, we are going to make it more generic using "SHELL" variable, instead of going for "bash -c" (when checking the version ...

One more thing, we made it work with your command (we aimed for "conda activate cryolo") but I don'0 t see an easy way to do it. What doe the "." do, without it, it fails!!

[delarosatrevin]

@pconesa for the future we should review the prepending of SCIPION_HOME for variables...I think it is quite confusing. The '.' is a more general way of 'source' command. What do you mean you aimed for 'conda activate cryolo'? I fine with this as default, but we should be flexible to have other activation commands, not strictly this one.

[delarosatrevin]

I just remembered another annoying thing...the validateInstallation is being called whenever you open a project...I think we need to review this...this it can slow down everything...in this case we are creating some processes.

[pconesa]

All validate installations are called when opening a project. It is true that in this case, the validation implies the execution of a process. When you say annoys you, you mean from a developer point of view, right? As a user, at least now it is not noticeable. This is used to visually, see if there is something wrongly configured. Any protocol which it's installation does not validate, will have a gray icon in the "all" view. I must agree that if this types of validations grow, it could delay the load, not sure how much though. Maybe, for the future, we can have a "Installation check window" to show what is wrongly configured and avoid the validate installation at load time.

As for the "conda activate cryolo" as a deafult command, it will work if we write it in a .sh (as it were before) and our command change to be "bash -ic runCryolo.sh". A plain runjob( "conda activate cryolo", args) will not create a logged shell (like with bash -i) and conda does not work. At least this it what we've seen. May be you know a way to do this within scipion, I mean make a runJob with a clean shell but with the bashrc executed.

[delarosatrevin]

@pconesa I think this validations and error messages can also be annoying for users...for example when Scipion is installed centrally and users does not have the option to change the installation/configuration. Imagine a user that never use cryOLO, why bothering with these (usually very cryptic for users) message about installation/configuration?

I still do not get the issue on your side with bash/conda activation/cryolo activation...still, if it requires bash on your side, why not to create a cryolo.bashrc in your installation, instead of pushing the SHELL logic into Scipion code?

PS: Cryolo 1.3.4 is out!

[pconesa]

Regarding bash: My guess is not something on our side, but something that is not possible to do in Scipion without a "xx.sh" only using runjob.

[delarosatrevin]

Ok, I'm a bit lost with this, maybe we should talk briefly and check how this issue affect this specify case for getting the cryolo version for validation.

[delarosatrevin]

@pconesa Any progress with this PR? It has been 2 weeks since our last talk about this.

[pconesa]

Actually the problem is not generic, and I shared my concerns about this on slack. Got no reply. We are al busy. Is the next thing in our list. But we have already done some evaluation.

On Thu, 9 May 2019, 11:54 Jose Miguel de la Rosa Trevin, < notifications@github.com> wrote:

@pconesa https://github.com/pconesa Any progress with this PR? It has been 2 weeks since our last talk about this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/scipion-em/scipion-em-sphire/pull/1#issuecomment-490838377, or mute the thread https://github.com/notifications/unsubscribe-auth/AAF7ZYID7RINVCJZPU5H25TPUPYETANCNFSM4HESIJHQ .

[pconesa]

@delarosatrevin we (@the-best-elephant - Peter) and I, with Ruben's ideas, found a nice and clean way to run, at least cryolo (not sure if this approach will work for other, potentially more complex conda environments).

The main change is:

Forget about CRYOLO_ENV_ACTIVATION and instead, use "standard" Scipion approach for other plugins defining CRYOLO_HOME

CRYOLOHOME: should point to the folder the python, and cryolo*.py scripts are.

Turns out that (this is ruben's experience) launching the python of the environment should work. I do not have much experience with conda, but tests are passing.

And we are able to validate the installation in the normal way.

[pconesa]

I thinkwe can do the same approach for NYSBC, @azazellochg do you mind if we adapt it and make a PR for the NYSBC? This way we will be consistent when interacting with conda plugins.

[delarosatrevin]

@pconesa I don't like this...I think the activation command is more general. The problem is that the "standard" HOME approach in Scipion for other plugins, is not standard or general enough. For example, the modules system. In some supercomputing environments, you can do something like 'module load relion/3.0.2', but this could have more complex definition than only setting the PATH and other environment variables. I guess the same could happen for conda, if the activation command set other more complex stuff that we don't know about. I'm fine if you prefer to go for the HOME approach and consider it more suited for you, but let's design a way where an activation command is still possible and we both will be happy.

[pconesa]

Thanks @delarosatrevin , we are keeping then your approach.

[delarosatrevin]

Thanks @pconesa. I don't want to impose my approach...just raising some concerns about flexibility and trying to be open to different scenarios.

[delarosatrevin]

Any plans when this PR will be merged? crYOLO 1.3.6 is out by now. :(

[azazellochg]

I dont have time to look at this during next two weeks