search

ToothAndClaw / SlicerAuto3dgm

Repo for auto3dgm slicer extension
2 stars 4 forks source link

Building of extension on the Slicer "factories" is blocking due to use of confirmOkCancelDisplay #6

Open jcfr opened 2 years ago

jcfr commented 2 years ago

This was observed on the windows factory (aka build machine) overload:

image

https://github.com/ToothAndClaw/SlicerAuto3dgm/blob/13cd3d98607c4ecddb45c7bdc02cb76569dd89bc/Auto3dgm/Auto3dgm.py#L68-L77

ebrahimebrahim commented 2 years ago

I'd be happy to look into this since it is similar to the dependency installation approach I took here in LungAIR.

But I'm not familiar with the Slicer extension building process, so I may not be of much use. e.g. I don't know why this code would be executed during extension building.

ebrahimebrahim commented 2 years ago

Summary of our live discussion.

The main problem here is that the confirm dialog causes tests to block, time out, then fail.

Approach to resolving this: make confirmOkCancelDisplay be smart enough to automatically choose an answer if it's called as part of testing. That answer would probably be "Ok" by default, but maybe the default should be something that can be chosen (with "Ok" being the default default)

Additionally, since it is common to have this dialog exchange with the user when pip installing, we may want to add an option to slicer.util.pip_install that streamlines the confirmation. However, we're not totally confident that this aligns best with the way people would want to use pip_install... so maybe it'd be good to first gather feedback on whether this is a desirable feature.

How can we tell if we are currently part of a testing situation? See this for a hint, but maybe some more digging is needed.

muratmaga commented 2 years ago

We do this in SlicerMOrph too for the open3d pckage, but we just do it as an acknowledgement as oppose to waiting for confirmation. https://github.com/SlicerMorph/SlicerMorph/blob/b5159aeb4ef291926f41f98ef2dc83097a3b87fc/ALPACA/ALPACA.py#L54

perhaps auto3dgm can also also use that approach? Otherwise I am not sure why SlicerMorph doesn't hang the extension build process but auto3dgm does.

jcfr commented 2 years ago

re: SlicerMOrph too for the open3d pckage

It looks like SlicerMorph also uses of slicer.util.confirmOkCancelDisplay expecting the user to click OK.

See https://github.com/Slicer/Slicer/blob/10fc1df859fa094691e2358d6c0b420bce477700/Base/Python/slicer/util.py#L2351-L2358

ebrahimebrahim commented 2 years ago

I am not sure why SlicerMorph doesn't hang the extension build process but auto3dgm does.

@jcfr Do you see why this^^ is the case then?

jcfr commented 2 years ago

It seems the corresponding SlicerMorph test (py_nomainwindow_qSlicerALPACAModuleGenericTest) also times out:

image

See https://slicer.cdash.org/viewTest.php?onlyfailed&buildid=2576902

ebrahimebrahim commented 2 years ago

Oh, good catch!

(I didn't realize we could access CDash for SlicerMorph, where is this linked? e.g. not seeing it listed here.)

jcfr commented 2 years ago

where is this linked?

Extensions build results are respectively submitted to SlicerPreview or SlicerStable dashboards in a dedicated group called Extension-Nightly

More details are available at https://slicer.readthedocs.io/en/latest/developer_guide/extensions.html#continuous-integration

ebrahimebrahim commented 2 years ago

I think this can be closed due to Slicer/Slicer#6162 :tada:

jcfr commented 2 years ago

Ditto. Thanks again @ebrahimebrahim :100:

Worth noting that since the extension is also being testing against 4.11, see https://github.com/Slicer/ExtensionsIndex/blob/4.11/Auto3dgm.s4ext

Could you backport a minimal version of the path against this branch: https://github.com/Slicer/Slicer/tree/v4.11

Once done, I will manually checkout the changes on the different factories.

All of that said, since we will be releasing soon ... we could also ignore that for now and let the tests time out.

ebrahimebrahim commented 2 years ago

Could you backport a minimal version of the path against this branch: https://github.com/Slicer/Slicer/tree/v4.11

Sure! I understand this to mean that I should submit my minimal patch as a PR on the Slicer repository, but requesting to merge with the v4.11 branch rather than with master. Please let me know if I'm not understanding correctly.

Once done, I will manually checkout the changes on the different factories.

I do not know what "different factories" refers to here.