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

[Feature] Add priority to tasks #427

Closed
maxim-uvarov opened this issue Apr 4, 2023 · 19 comments
Closed

[Feature] Add priority to tasks #427

maxim-uvarov opened this issue Apr 4, 2023 · 19 comments
Assignees
Labels
s: Client This issue touches the pueue client s: Daemon This issue touches pueue daemon s: Pueue-lib This issue touches the pueue-lib library t: Feature A new feature that needs implementation

Comments

@maxim-uvarov
Copy link

A detailed description of the feature you would like to see added.

Please consider adding an option to the queue tasks to be executed immediately after the tasks that are currently running, without exceeding the current the parallel limit (as it would with the pueue add -i).

Explain your usecase of the requested feature

I use Pueue as a queue manager for downloading files from the IPFS network. Typically, I have a long list of files that are added to the queue after several unsuccessful attempts. However, I want to be able to add the command to download new files with a higher priority. Unfortunately, I cannot use pueue add -i because I receive new CIDs in large batches.

Alternatives

No response

Additional context

No response

@maxim-uvarov maxim-uvarov added the t: Feature A new feature that needs implementation label Apr 4, 2023
@Nukesor
Copy link
Owner

Nukesor commented Apr 5, 2023

I guess a simple "-o/--priority" option should be still in scope.
This needs a bit of care to maintain backwards compatibility, but otherwise this shouldn't be too hard to implement.
Make sure to properly test and document the changes in the scheduling logic, though.

I won't work on this myself for now, as I'm quite short on time recently, so feel free to implement this yourself :)

@maxim-uvarov
Copy link
Author

maxim-uvarov commented Apr 5, 2023

I guess a simple "-o/--priority" option should be still in scope. This needs a bit of care to maintain backwards compatibility, but otherwise this shouldn't be too hard to implement. Make sure to properly test and document the changes in the scheduling logic, though.

I won't work on this myself for now, as I'm quite short on time recently, so feel free to implement this yourself :)

Thank you for your response! I'm not a Rust programmer yet, but there's a chance that I might become one soon. I will check if I can handle it. 🙏

@Nukesor
Copy link
Owner

Nukesor commented Apr 6, 2023

If you don't find the time, don't worry.

I'll get to it eventually^^

@Nukesor Nukesor changed the title add to queue immediately after the running task [Feature] Add priority to tasks Apr 15, 2023
@Nukesor Nukesor self-assigned this May 22, 2023
@Nukesor Nukesor added s: Pueue-lib This issue touches the pueue-lib library s: Daemon This issue touches pueue daemon s: Client This issue touches the pueue client labels May 22, 2023
@Nukesor
Copy link
Owner

Nukesor commented May 27, 2023

FYI, I started working on this :)

@Nukesor
Copy link
Owner

Nukesor commented May 27, 2023

Ok. I need a bit of input.

I'm not 100% sure how to design the priority field on the task.
I started to implement it in a rather naive way:

  1. A task's priority can be in the range of [0]-[100].
  2. task.priority is None by default of type Option<usize>.
  3. If a task has any priority (including 0), it has a higher priority than a task with priority None.
  4. Tasks with bigger priorities are more important.

What it's good for

This works for the use-case that a user knows that a more important/less important tasks might get queued and gives them a "middle" priority by default of let's say 50.
It also works for users that don't know about priorities or didn't prepare and want to schedule an important task. That would also work rather well.

What doesn't work

Now I came up with the use-case of a user that didn't know about priorities or didn't prepare and wants to queue a task that should be processed at a later time (or at the very end).
This would not be possible with the approach mentioned above

An alternative approach

  1. A task's priority can be in the range of [-100] - [100].
  2. task.priority is 0 by default of type usize.
  3. Tasks with bigger priorities are more important.

I'm not sure if negative priorities are a good design decision, but it would cover all scenarios I can come up with.

What do you think?

@Nukesor Nukesor mentioned this issue May 28, 2023
@Nukesor
Copy link
Owner

Nukesor commented May 28, 2023

I implemented the feature as described in the "alternative approach".
It can be found over here: #440

In case you installed pueue via cargo, you can test the branch like this:

git clone git@github.com:Nukesor/pueue
cd pueue
git switch task-priority
cargo install --path pueue 

@maxim-uvarov
Copy link
Author

I implemented the feature as described in the "alternative approach".

Wow! thank you so much!

I have tried to build this version, but got 17 errors, like in code below.
I asked ChatGPT what can I do, and it said:

To fix the issue, you may need to adjust the code to use the correct attribute type from the `crossterm` crate or update the code to use the correct crate that provides the attribute required.

But I don't know how to do it.
Maybe you can tell me?

error[E0308]: mismatched types
   --> pueue/src/client/display/group.rs:31:73
    |
31  |     let name = style.style_text(format!("Group \"{name}\""), None, Some(Attribute::Bold));
    |                                                                    ---- ^^^^^^^^^^^^^^^ expected enum `crossterm::style::Attribute`, found enum `comfy_table::Attribute`
    |                                                                    |
    |                                                                    arguments to this enum variant are incorrect
    |
    = note: enum `comfy_table::Attribute` and enum `crossterm::style::Attribute` have similar names, but are actually distinct types
note: enum `comfy_table::Attribute` is defined in crate `crossterm`
   --> /Users/user/.cargo/registry/src/github.com-1ecc6299db9ec823/crossterm-0.26.1/src/style/types/attribute.rs:92:1
    |
92  | / Attribute! {
93  | |     /// Resets all the attributes.
94  | |     Reset = 0,
95  | |     /// Increases the text intensity.
...   |
153 | |     NotOverLined = 55,
154 | | }
    | |_^
note: enum `crossterm::style::Attribute` is defined in crate `crossterm`
   --> /Users/user/.cargo/registry/src/github.com-1ecc6299db9ec823/crossterm-0.25.0/src/style/types/attribute.rs:92:1
    |
92  | / Attribute! {
93  | |     /// Resets all the attributes.
94  | |     Reset = 0,
95  | |     /// Increases the text intensity.
...   |
153 | |     NotOverLined = 55,
154 | | }
    | |_^
    = note: perhaps two different versions of crate `crossterm` are being used?
note: tuple variant defined here
   --> /rustc/d5a82bbd26e1ad8b7401f6a718a9c57c96905483/library/core/src/option.rs:526:5

@maxim-uvarov
Copy link
Author

In case your first question is still valid:
I like the alternative approach, but I would choose the range [0..100] and assign 50 to tasks by default. I think this would be better than allowing negative priorities.
But it is just a matter of taste.

@Nukesor
Copy link
Owner

Nukesor commented May 28, 2023

Ah fuck. Sorry I forgot to add the --locked to the install path.

cargo install --path pueue --locked

One of the ugly remainders of the first versions of Rust. install allows updated dependencies and doesn't respect lockfiles -.-. I constantly stumble over this one.

@maxim-uvarov
Copy link
Author

cargo install --path pueue --locked

won't work either

image

@Nukesor
Copy link
Owner

Nukesor commented May 28, 2023

Huh. That's weird.
Could you pull again, I rebased the branch and it should now work since I pinned the dependencies on the development branch.

Sorry for the inconvenience. I accidentally introduced a semver breaking change in one of my other crates, which is used by Pueue. This is the aftermath of that mistake.

If you pull now, you should however see the prio column show up in the status command, if there's a non-default priority set :) I just added that feature a minute ago.

@maxim-uvarov
Copy link
Author

maxim-uvarov commented May 28, 2023

Thank you! Now it's been builded.

I tried it, but seems like priority or hasn't been set, or reported incorrectly. The first is likely, as the task hasn't been finished yet.
image

@maxim-uvarov
Copy link
Author

maxim-uvarov commented May 28, 2023

P.S. It's late in my timezone. I'm going to sleep. I will respond during the week. Thank you!
image

@Nukesor
Copy link
Owner

Nukesor commented May 28, 2023

You used the wrong parameter :)

The -p flag was already used by the --print-job-id, which is why the --priority flag has the -o shorthand.

@maxim-uvarov
Copy link
Author

maxim-uvarov commented May 29, 2023

Unfortunately, none of them worked in my case

image image

@Nukesor
Copy link
Owner

Nukesor commented May 29, 2023

As you can see, you put the -o flag behind the command to be executed.

The command is echo "4002" -o 50

You probably executed pueue add echo "4002" -o 50 instead of pueue add -o 50 -- echo "4002"

@Nukesor
Copy link
Owner

Nukesor commented May 29, 2023

Anyhow, I already tested, added good test coverage it and I'm pretty sure it works as expected.

Don't bother trying to get it running, I'll merge as is.

@Nukesor Nukesor closed this as completed May 29, 2023
@maxim-uvarov
Copy link
Author

I am sorry for my inattention.
Thank you for the feature!

@maxim-uvarov
Copy link
Author

maxim-uvarov commented May 30, 2023

Actually, I use pueue in my script.
I don't have much another everyday tasks where I can use it. And because of it - I don't know by heart how to use it. For sure I needed to consult the help, but ...

P.S. I saw your comit with clarification, that Pueue is not for scripts, though as far as I can see it, Pueue suits my goals well enough and I am going to continue using it.

Thank you again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: Client This issue touches the pueue client s: Daemon This issue touches pueue daemon s: Pueue-lib This issue touches the pueue-lib library t: Feature A new feature that needs implementation
Projects
None yet
Development

No branches or pull requests

2 participants