Skip to content
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

support grass algorithms in profile folder #53246

Merged
merged 10 commits into from
Jul 6, 2023
Merged

Conversation

AlisterH
Copy link
Contributor

@AlisterH AlisterH commented May 26, 2023

Description

Allow users to create algorithm descriptions in [QGIS profile folder]/processing/grassaddons/description.
An addon shipped with QGIS can also be overridden by creating a modified version here.
The ext mechanism is supported at [QGIS profile folder]/processing/grassaddons/ext.

This follows on from #53049, to implement #53048.

@github-actions github-actions bot added the GRASS label May 26, 2023
@github-actions github-actions bot added this to the 3.32.0 milestone May 26, 2023
@AlisterH
Copy link
Contributor Author

AlisterH commented May 28, 2023

I have tried to implement this as simply as possible; not sure what someone more competent would think.
We should probably do a couple more things:

  • create [QGIS profile folder]/processing/grassaddons/ext
  • put a copy of python\plugins\grassprovider\grass7.txt in [QGIS profile folder]/processing/grassaddons/

But I guess the big question is whether [QGIS profile folder]/processing/grassaddons/ is the best place to use.

@nicogodet
Copy link
Member

An addon shipped with QGIS can also be overridden by creating a modified version here

Looking at current implementation, it seems that user defined description will be added first then shipped desc and override user one ?

createAlgsList can return duplicated alg and it may lead to unwanted behavior. It may be worth checking is alg exists before adding it to list and raise a warning.

@AlisterH
Copy link
Contributor Author

AlisterH commented May 30, 2023

I believe this is correctly handled by qgsprocessingprovider.cpp
It raises a warning "Duplicate algorithm name..."
And algorithms in [QGIS profile folder]/processing/grassaddons seem to override those in python\plugins\grassprovider\ as desired.

Do you want an additional check anyway?
I guess it would make the provider load a bit faster, in case someone wanted to override a large number of algorithms.

@AlisterH
Copy link
Contributor Author

When I say "override" I mean "are loaded in preference to".

spec = importlib.util.spec_from_file_location('grassprovider.ext.' + name, extpath)
self.module = importlib.util.module_from_spec(spec)
spec.loader.exec_module(self.module)
except Exception:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we avoid the broad exception here and catch specific ones instead?

Copy link
Contributor Author

@AlisterH AlisterH May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any advice on what to catch, given that we're loading code written by the user?

  1. Maybe we just need to be more informative?:
        except Exception as e:
            QgsMessageLog.logMessage(self.tr('Failed to load: {0}\n{1}').format(extpath, str(e)), 'Processing', Qgis.Critical)
            pass
  1. Are you happy with pass rather than raise e? This means if someone has written a working algorithm description, in most cases it will be useable even if they are struggling with getting an ext module module they're writing to work.
  2. Should Qgis.Critical be downgraded? With the current pull request we get a Qgis.Critical Could not open GRASS GIS 7 algorithm:, with the exception, so I presume we should stick with Qgis.Critical

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with a QgsMessage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added that to the pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did wonder if we should add something like this, but it didn't seem right when we don't show a message even when we fail to load an algorithm:

            iface.messageBar().pushMessage("Warning", "GRASS algorithm " + self.name() + " failed to load ext module - see Processing log messages", 1)

python/plugins/grassprovider/Grass7AlgorithmProvider.py Outdated Show resolved Hide resolved
AlisterH and others added 2 commits May 31, 2023 21:36
Co-authored-by: Nicolas Godet <39594821+nicogodet@users.noreply.github.com>
Co-authored-by: Nicolas Godet <39594821+nicogodet@users.noreply.github.com>
@github-actions
Copy link

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jun 20, 2023
@AlisterH
Copy link
Contributor Author

Bumping to stop the bot killing us.

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jun 20, 2023
@github-actions
Copy link

github-actions bot commented Jul 5, 2023

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 5, 2023
@AlisterH
Copy link
Contributor Author

AlisterH commented Jul 5, 2023

Bumping again to stay alive.

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 5, 2023
@nyalldawson nyalldawson merged commit 75420de into qgis:master Jul 6, 2023
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants