-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Decorators to make processing scripts easier #134
Comments
@nyalldawson early QEP for this as I flesh out more details ping @anitagraser @luipir @wonder-sk @m-kuhn and anyone else who is keen. |
Looks beautiful to me! |
Nice work, great idea! |
@nyalldawson how do you feel about this
I wonder if we shouldn't use
|
Looking at it though I think I do still like the Feelings on that? |
We definitely need the distinction here -- otherwise it's impossible e.g to differentiate a string parameter which a user has to enter from a string value generated by the algorithm itself. |
Yep agree. Leaving it as is.
…On Wed, 28 Nov. 2018, 8:11 am Nyall Dawson ***@***.*** wrote:
We definitely need the distinction here -- otherwise it's impossible e.g
to differentiate a string parameter which a user has to enter from a string
value generated by the algorithm itself.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#134 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAXS3Oio_2QofBVqI5SaP_IVvciORZ0xks5uzbiHgaJpZM4YyQ0E>
.
|
what would be the expected values for Type? would be the value from https://qgis.org/api/classQgsProcessingParameterDefinition.html#a9f99ab59bf4bcc1ff7cc8fe389cd6721 ? |
@luipir yeah so far is done like this
|
with those make functions looking like this:
etc |
@NathanW2 good idea! As someone which has not created an algo for some time: First param (of @alg) is id and second comment? Would it be an idea to also add (optional, eg in examples) keywords for those? Just to make it superclear for newbies? Or is that too much? Should we also (force to?) add a description? Both to algo itself as as part of input/outputs (preferably translatable off course ;-)? Thanks Nathan for this already! |
@rduivenvoorde Yep I will update some of the examples to show what is possible and will throw exceptions if there is no description set. |
Implemented in: qgis/QGIS#8586 |
As part of implementing the changes the workshop script goes from 115 lines down to 50 using this new method. Including the changes from @nyalldawson to loop over parts easier. |
thx @NathanW2. It resembles a lot the way params were defined in QGIS2, but now it's true, standard, code. Have you ever considered the "builder" pattern (maybe with chaining)? |
Do you have an example of what you mean?
…On Mon, 3 Dec. 2018, 7:50 pm Giovanni Allegri ***@***.*** wrote:
thx @NathanW2 <https://github.com/NathanW2>. It resembles a lot the way
params were defined in QGIS2, but now it's true, standard, code.
Have you ever considered the "builder" pattern (maybe with chaining)?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#134 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAXS3DCd4wBO6ObPy_mVRXkIyZf2ZiRaks5u1PPvgaJpZM4YyQ0E>
.
|
something like:
and if you return the builder instance on each method you can have chaining:
it seems cleaner to me then that long list of decorators, and the validation can be done at the builder build() stage. |
Personally, I don't find that much clearer really and it just moves it
someplace else and detaches it from the function.
Validation is already done inside "@alg" wrapper on the final call to check
for input and outputs, etc so it really comes down to the same.
…On Mon, Dec 3, 2018 at 8:49 PM Giovanni Allegri ***@***.***> wrote:
something like:
params= alg.ParamsBuilder()
params.addInput(type=alg.FEATURE_SOURCE, name="INLAYER", label="Input layer")
params.addOutput(type=alg.SINK, name="OUTPUT", label="Output layer")
params.build()
@alg(name="test", label="Test script", group="Workshop", groupid="workshop",parameters=params)
def my_alg(instance, parameters, context, feedback):
return {"OUTPUT": dest_id}
and if you return the builder instance on each method you can have
chaining:
params= alg.ParamsBuilder()
params.addInput(type=alg.FEATURE_SOURCE, name="INLAYER", label="Input layer")
.addOutput(type=alg.SINK, name="OUTPUT", label="Output layer")
(....)
it seems cleaner to me then that long list of decorators, and the
validation can be done at the builder build() stage.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#134 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAXS3L1kM2LsWz5fw-G7ILOhq2mPwi-oks5u1QGjgaJpZM4YyQ0E>
.
|
The only advantage I see to use the Builder pattern instead of the Decorator one (that is also a GoF pattern) respect Builder one is that params constructor can be inherited... IMHO in 99% of cases this feature is not used,. The only code I saw that can use this feature I found in LAStool processing provider to specify base generic parameter, but I suspect that the introduction of this pattern wouldn't trigger a refactoring of providers. I'm for +0 |
in reality, both methods can be used in future here if the need comes up.
No reason the @alg() function can't take a builder as a option I just don't
like it for the simple case this is trying to solve. If there is a complex
problem it would solve we can have a look at the, but there might be cases
when you have to use the full normal class and not this shortcut method.
This is simply to cover the easy case, anything complex will still need the
extra logic the class needs.
…On Mon, Dec 3, 2018 at 9:37 PM Luigi Pirelli ***@***.***> wrote:
The only advantage I see to use the Builder pattern instead of the
Decorator one (that is also a GoF pattern) respect Builder one is that
params constructor can be inherited... IMHO in 99% of cases this feature is
not used,. The only code I saw that can use this feature I found in LAStool
processing provider to specify base generic parameter, but I suspect that
the introduction of this pattern wouldn't trigger a refactoring of
providers.
@giohappy <https://github.com/giohappy> are you planning to develop
complex providers that would have advantage of the proposed pattern?
I'm for +0
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#134 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAXS3NxfAt187LjoRTB7yB1fUf7Kj6Thks5u1QzkgaJpZM4YyQ0E>
.
|
Nice! Two minor comments (sorry for arriving late at this): -Why an exception if an output is not defined? That is still a valid algorithm... Awesome work! |
@volaya I discussed this with @NathanW2 prior to merge -- but the thinking here is that EVERY algorithm should have at least a single output. Even if the algorithm doesn't create a value, it still should be outputting something -- e.g. if it's a "delete file" algorithm, it should output the name of the file deleted, or whether the file was successfully deleted. I can't think of any algorithms which shouldn't have outputs. |
@volaya - oops, sent early. Was going to also say that we really want to encourage developers to add as MANY outputs as possible to algorithms, because the more outputs are available, the more powerful expressions can become which utilise these output values. |
To be able to create Python scripts in a simple way has been a great asset for users of QGIS 2. Without this wrapper, one has to extend QgsProcessingAlgorithm, and that will be a barrier for most of our users. It would therefore be very convenient to have this included in the LTR (3.4). Is that possible? |
Any objections to me backporting this into 3.4 LTR next Monday? |
@NathanW2 Yes - I'd like to delay this for at least one cycle so that we've got time to cement the API before making it widely available. E.g. we ideally need a solution to allow use of these simpler algorithms within a custom provider instead of always being inside the script provider. I think there may also be an issue with forcing outputs and not detecting that some inputs also automatically create outputs (e.g. a sink input should be sufficient on its own). But I'll keep investigating that one... I'd rather take it slow and make the API rock solid first. |
No problem. I'm in no rush to backport it if you don't think it's not
ready yet.
…On Wed, Jan 30, 2019 at 9:35 AM Nyall Dawson ***@***.***> wrote:
@NathanW2 <https://github.com/NathanW2> Yes - I'd like to delay this for
at least one cycle so that we've got time to cement the API before making
it widely available.
E.g. we ideally need a solution to allow use of these simpler algorithms
within a custom provider instead of always being inside the script
provider. I think there may also be an issue with forcing outputs and not
detecting that some inputs also automatically create outputs (e.g. a sink
input should be sufficient on its own). But I'll keep investigating that
one...
I'd rather take it slow and make the API rock solid first.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#134 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAXS3BgZCEvLrBK7KS-_SyPIgqqSrm0-ks5vINrEgaJpZM4YyQ0E>
.
|
Is the exception on missing output parameter actually implemented now? I've stripped the above example down to the bare minimum template (without output, to mimic the template I had before: https://anitagraser.com/2019/03/02/easy-processing-scripts-comeback-in-qgis-3-6/) and don't see an exception anywhere. |
Working on the documentation (https://docs.qgis.org/testing/en/docs/user_manual/processing/scripts.html#the-alg-decorator), I tried to use "parent" when declaring an alg.DISTANCE parameter:
where 'INPUT' is declared as an alg.SOURCE. This resulted in a Python error when trying to save the script from the Processing Script Editor:
with QGIS 3.6.2 on Ubuntu 18.04. When extending QgsProcessingAlgorithm it is possible to declare a parent for distance parameters:
Is this possible for alg.DISTANCE parameters (when using the |
Thank you for this great approach! I find it way more intuitive than Extending QgsProcessingAlgorithm. I have a small question regarding the order the inputs are shown in the GUI. Is there a way to determine which @alg.input will be shown first in the GUI-Window? Right now, it seems totally random to me and it can be really counterintuitive if several input numbers are asked before an input folder. Anyways, great work! |
Thank you for this enhancement. Is it already possible to pass a I wrote a short script that outputs data in GPX-format only, so I tried something like this, which also gives me an 'unknown keyword argument'-message:
|
QGIS Enhancement: Decorators to make processing scripts easier
Date 2018/11/26
Author Nathan Woodrow (@NathanW2 )
Contact woodrow.nathan at gmail dot com
maintainer @NathanW2
Version QGIS 3.6
Summary
While preparing a workshop to cover creating a custom processing script it became evident that there was a lot of code just to get a script written when a lot of it was boilerplate that could be avoided.
Most of the meat of the script is inside the
initAlgorithm
andprocessAlgorithm
methods, however, there is a lot of code around these methods to just get them to work correctly and people get lost easy.The overall goal is a full custom script on a single slide to make it easy to demo.
Proposed Solution
New Python decorators that will streamline the process of creating a custom script method to make it easier to read and understand what is going on. Decorators are also more native to Python and fit this use case well.
Example(s)
Example of multi outputs:
The above decorators will create a QgsProcessingAlgorithm instance ready to use. This will be used in the script editor and added the providers like normal.
Defines a new QgsProcessingAlgorithm
@alg.input(type=alg.FEATURE_SOURCE, name="INLAYER", label="Input layer")
Each
input
will take the type as the first arg which is translated into the correctQgsProcessingParameter*
type when called. Overall goal here is to make it easy to understand for new users and feel more Python and less C++@alg.output(type=str, name=alg.NAMES.OUT2, label="Output")
Defines an output type. After discussions with @nyalldawson we think it's best to have at least one out always defined to avoid fully black box algorithms if none is set. If a output isn't defined it will raise an exception.
Define parent inputs using:
parent="INLAYER"
this will check against all inputs to make sure it has already been defined.Keyword arguments
All args and keyword arguments will be passed down though into the correct
QgsProcessingParameterDefinition
meaning@alg.input
can accept the same keywords asQgsProcessingParameterDefinition
for that typeNote: The following args are changed to match the C++ args inside the wrapper:
Just to make it easier to use.
Example:
will work like the following
Error handling
The decorators will raise exceptions for a range of different issue to help users debug issues.
Some example exceptions:
Affected Files
New
processing
module inpython\qgis
folder for processing so we can doand expose everything through there as stable and public API. At the moment you have to import the processing plugin code which really should be internal only.
New
AlgWrapper
andProcessingAlgFactory
which creates and manages the current instance being created. These can only be created on the main thread so no risk of race conditions here hopefully.The basic shape of
ProcessingAlgFactory
is this however this will change as the code evolves. The example below is just how the core of it works:define()
is the main entry point to create a new instance of the wrapper object, which is also part of__call__
in order to allow this:The inner function of
define()
will be called at the end so we use that to calll the final methods on the wrapper and make sure we have all the input and outputs defined correctly.Examples
Performance Implications
(required if known at design time)
Further Considerations/Improvements
(optional)
Backwards Compatibility
No compatibility issues as only a wrapper over normal logic.
Issue Tracking ID(s)
qgis/QGIS#8586
Votes
(required)
The text was updated successfully, but these errors were encountered: