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

Add OTB provider to qgis processing plugin [processing] #8814

Merged
merged 15 commits into from Feb 22, 2019

Conversation

Projects
None yet
8 participants
@rkanavath
Copy link
Contributor

rkanavath commented Jan 8, 2019

Description

This adds otb provider to qgis processing plugin. Here are some highlights.

  • OTB descriptor files are not included/maintained in qgis source tree.
  • OTB provider is generic and works for all OTB version starting from 6.6.0
  • Testing of OTB algorithms has been added (using .yaml and others).
  • No changes into qgis core other than a python import in Processing.py.
  • Travis ci script (linux) is added to install otb package. (osx will be added if linux ci pipeline passes)

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

@rkanavath rkanavath force-pushed the orfeotoolbox:master branch from 97a9496 to c674faf Jan 9, 2019

@rkanavath rkanavath force-pushed the orfeotoolbox:master branch from 8b4f28a to 2bbb911 Jan 10, 2019

@rkanavath rkanavath changed the title Add OTB provider to qgis processing plugin [processing] Add OTB provider to qgis processing plugin Jan 10, 2019

@rkanavath

This comment has been minimized.

Copy link
Contributor Author

rkanavath commented Jan 10, 2019

@luipir , could you have a look at changes. travis tests are fixed now

@luipir

This comment has been minimized.

Copy link
Contributor

luipir commented Jan 10, 2019

I'll give a try... but I'm busy on many front (userconf) in this moment. No problem if someone else is able to review before me

@nyalldawson nyalldawson added the Feature label Jan 10, 2019

@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson commented Jan 10, 2019

I'd like to review before merge too, thanks!

But upfront: Thanks @rkanavath --- looks like great work from a preliminary read over 🎉

@luipir

This comment has been minimized.

Copy link
Contributor

luipir commented Jan 13, 2019

But upfront: Thanks @rkanavath --- looks like great work from a preliminary read over

of course, thanks @rkanavath :)

@rkanavath rkanavath force-pushed the orfeotoolbox:master branch from 2334407 to c47c7e9 Jan 14, 2019

@m-kuhn m-kuhn changed the title [processing] Add OTB provider to qgis processing plugin Add OTB provider to qgis processing plugin [processing] Jan 16, 2019

@nyalldawson nyalldawson added this to the 3.8 milestone Jan 18, 2019

@fjperini fjperini referenced this pull request Jan 20, 2019

Closed

Add OTB6 support to QGIS #431

@rkanavath

This comment has been minimized.

Copy link
Contributor Author

rkanavath commented Jan 21, 2019

@nyalldawson , given that last commits are okay is it possible to move this to 3.6 milestone?

@pcav

This comment has been minimized.

Copy link
Member

pcav commented Jan 22, 2019

so the plugin is not trying to install, the use can do it in his usual way. thanks.

@rkanavath

This comment has been minimized.

Copy link
Contributor Author

rkanavath commented Jan 22, 2019

@pcav , yes plugin know nothing of how or if otb is installed. The change in travis docker is for testing in travis-ci.

@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson commented Jan 23, 2019

This looks good to me now. There's going to be some issues with interoperability with other algorithms in models due to the layer handling (i.e. not supporting memory layers), but that can always be addressed later and further refined.

@alexbruy did you want to review too?

There's still the outstanding question about the docker image changes too, which either @3nids or @elpaso will need to review

@SrNetoChan

This comment has been minimized.

Copy link
Member

SrNetoChan commented Jan 24, 2019

Wasn't it decided that except for GRASS and SAGA all the other providers would stay outside the Core, installed as plugins?

@rkanavath

This comment has been minimized.

Copy link
Contributor Author

rkanavath commented Jan 24, 2019

Thanks @nyalldawson and others for your help during review process. I really appreciate your inputs. Plus lot of great changes in processing which made code for provider much simpler!

I agree with you that in memory provider support can be added later. I will add this as a todo in otb bug tracker.

Regarding docker image changes, I don't have an alternative for testing otb. So will wait for inputs from @elpaso or @3nids.

rkanavath added some commits Jan 21, 2019

[test] new test for OtbChoiceWidget
This can easily go into GuiTests.py but we tend to keep it out due to
usage of create_from_metadata(). All widget in GuiTests uses
create_from_class() which will not work for this special widget
check for layer providers and authid
OTB only supports gdal and ogr providers for now. Maybe memory
provider can be easily supported using some conversion on the fly.
For the moment, we can go with this method. IO Formats in OTB not
using GDAL/OGR (LUM, ONERA) are not supported by QGis. Those can be
treated as simple files.

nyalldawson,  pointed that AUTHORITY id can have types not starting
with 'EPSG:'. Current otb takes just EPSG number and run with it. The
algorithm doesn't know what to with a number which is not EPSG because
it uses Gdal's 'ImportFromEpsg' method AFAIR.

QgsProecessing Exception is raised in both the above invalid cases.

@rkanavath rkanavath force-pushed the orfeotoolbox:master branch from 6c58195 to ba3d9f5 Jan 24, 2019

@alexbruy

This comment has been minimized.

Copy link
Contributor

alexbruy commented Jan 24, 2019

@nyalldawson, sorry I won't review it. My position did not changed, this should be a 3rd party plugin like all other Processing providers. I don't see any reasons for exceptions, we had agreements "no more python plugins in the core" and "move processing providers outside core".

@pcav

This comment has been minimized.

Copy link
Member

pcav commented Jan 24, 2019

@alexbruy I can't find it back right now, but we have been discussing this case rather extensively, and the final decision was to include OTB

@alexbruy

This comment has been minimized.

Copy link
Contributor

alexbruy commented Jan 24, 2019

@pcav I perfectly remember what and how was decided

@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson commented Jan 25, 2019

@alexbruy

That's fine -- I totally get where you are coming from. If there's no objection to the docker changes I'll merge after thaw.

@rkanavath

This comment has been minimized.

Copy link
Contributor Author

rkanavath commented Jan 30, 2019

@nyalldawson, @pcav is it possible for someone else to review changes to docker file in travis to complete it. If @elpaso (I tried to contact him by mail) and @3nids too are not agreeing (don't want to understand why is that) to include otb provider and thereby refusing to even review changes what will be remaning option left.?

Even though docker changes aren't essential for functioning of provider itself but it allows for easy testing and helps improve integration and communication between two projects.

Thanks in advance.

@pcav

This comment has been minimized.

Copy link
Member

pcav commented Jan 30, 2019

I agree. Unfortunately, I don't know the docker infrastructure, so I cannot be of direct help here.

@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson commented Jan 31, 2019

It's ok, it's tagged as "merge after thaw", so we are just waiting on feature freeze to lift and I'll merge

@pcav

This comment has been minimized.

Copy link
Member

pcav commented Feb 1, 2019

Great news, I'll test soon after merge.
Thanks.

@vpoughon

This comment has been minimized.

Copy link
Contributor

vpoughon commented Feb 1, 2019

Hi all,

It's great to see that this PR is going to be merged. Thanks for all the work!

I'm a developer and PSC member of the OTB project, just a quick note: Reading this thread I see that there is a bit of friction about where this code belongs. IIRC, the main reason we pushed for the OTB provider to be part of QGIS core is that we had a few issues with API breakage. When it was outside of core the plugin suddenly stopped working for users who upgraded their QGIS version. If you can find another reference to a discution about this please let me know.

On our side, integration with QGIS is an important feature. We see a lot of users relying on both of these osgeo projects and we just want what's best for them. For this we are willing to spend ressources and work on QGIS integration in the years to come (i.e. assign dev time to make QGIS PRs, or whatever else is the best technical solution).

There's also the issue of documentation, and installation of the plugin. Both of these are a complete mess from a UX perspective given the state of QGIS/OTB/plugin version compatibility matrix.

Ultimately I feel like the question is about who should maintain this code. I'm not well versed in the technical details of this (@rkanavath did all the work), but I just want to make clear that we are not dumping this code in the QGIS repo hoping that it will just keep working. We are open for any discussion necessary (both technical and political) about how to make the best OTB-QGIS plugin for users. We have a gitlab issue tracker and a new discourse forum, both with github SSO, or feel free to reply here.

Thanks a lot

@pcav

This comment has been minimized.

Copy link
Member

pcav commented Feb 1, 2019

Hi Victor,
thanks for stepping in. I assume that if you take responsibility for this area of code, there is no reason not to include it. I fully support your efforts.

@nyalldawson nyalldawson merged commit 9983961 into qgis:master Feb 22, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pcav

This comment has been minimized.

Copy link
Member

pcav commented Feb 23, 2019

Compiled and tested: it requires to add manually the paths to OTB folder and OTB application folder. Tried /usr nd /usr/bin, but I still get errors.

@pcav

This comment has been minimized.

Copy link
Member

pcav commented Feb 23, 2019

No OTB algorithms found in '/usr/bin'. OTB will be disabled

@pcav

This comment has been minimized.

Copy link
Member

pcav commented Feb 23, 2019

ls /usr/bin/otbcli*
/usr/bin/otbcli /usr/bin/otbcli_MorphologicalClassification
/usr/bin/otbcli_BandMath /usr/bin/otbcli_MorphologicalMultiScaleDecomposition
/usr/bin/otbcli_BinaryMorphologicalOperation /usr/bin/otbcli_MorphologicalProfilesAnalysis
/usr/bin/otbcli_BlockMatching /usr/bin/otbcli_MultiImageSamplingRate
...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment