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

Properly handle task limit #28

Closed
whilefoo opened this issue Aug 27, 2024 · 23 comments · Fixed by #91
Closed

Properly handle task limit #28

whilefoo opened this issue Aug 27, 2024 · 23 comments · Fixed by #91

Comments

@whilefoo
Copy link
Member

          The original implementation allowed for productive contributors to work on two tasks concurrently. With a starting two task limit:
Condition Task Adjustment Explanation
If any reviewer approved the pull +1 task per pull It's likely that the pull is good, just waiting on other slow reviewers to confirm.
If reviewers take longer than 24 hours +1 task per pull Don't wait for lazy reviewers to be able to start a new task.
If any reviewer requested changes -1 task per pull They should focus on addressing the changes as a top priority.

Originally posted by @0x4007 in #19 (comment)

For the sake of this conversation a completed PR is when the conditions are met in the above table.

In the current state the code checks if any review is approved or if there is 0 reviews but 24 hours have passed since the creation of the PR, however it doesn't check if there's any requested changes so it will count it as completed even if there 1 approve and 1 requested changes.

There is still a problem when the reviewer requests changes and the contributor resolves those changes but waits for the reviewer to make a new review so they can't start another task.
There's two possible solutions: check if the reviewer was requested more than x hours ago or check that changes have been marked as solved

@ariesgun
Copy link
Contributor

/start

Copy link

! You have reached your max task limit. Please close out some tasks before assigning new ones.

@jordan-ae
Copy link
Contributor

/start

Copy link

Warning! This task was created over 58 days ago. Please confirm that this issue specification is accurate before starting.
Deadline Fri, Oct 25, 12:38 AM UTC
Beneficiary 0x2F05fD58023B0a95d1866aa0A3b672cEf05945c5

Tip

  • Use /wallet 0x0000...0000 if you want to update your registered payment wallet address.
  • Be sure to open a draft pull request as soon as possible to communicate updates on your progress.
  • Be sure to provide timely updates to us when requested, or you will be automatically unassigned from the task.

Copy link

Passed the deadline and no activity is detected, removing assignees: @jordan-ae.

Copy link

! jordan-ae you were previously unassigned from this task. You cannot be reassigned.

Copy link

@jordan-ae the deadline is at Tue, Nov 5, 4:32 PM UTC

Copy link

A new workroom has been created for this task. Join chat

Copy link

Passed the deadline and no activity is detected, removing assignees: @jordan-ae.

@ishowvel
Copy link

Is this task up for grabs or is @jordan-ae still working on it

@jordan-ae
Copy link
Contributor

Still working on it trying to get test pass. Giving me a bit of a hassle

@kingsley-einstein
Copy link

/stop

1 similar comment
@kingsley-einstein
Copy link

/stop

@Keyrxng
Copy link
Contributor

Keyrxng commented Nov 21, 2024

This has a big chance of not performing well as a worker with these suggestions because:

  • for every assigned issue we need to paginate reviews (potentially 10s-100s) for each
  • this feature increases that limit more and more

Perhaps moving to a dist build action should be considered?

@gentlementlegen
Copy link
Member

gentlementlegen commented Nov 22, 2024

@Keyrxng While I do agree that it will elongate the run time, I am not sure if we need to paginate reviews? I didn't go through the whole API but maybe we can only consider the last review? Or get a list of unresolved comments? I actually am worried more about hitting the fetch limit that the runtime duration itself (because runtime duration can be optimized, api are much harder to improve).

Moving to a dist build is totally possible, but the main problem is that even if the run could be instantaneous, we cannot skip the duration where the action run is being picked up by a gitHub node. This can sometimes take 1s, sometimes 5 minutes or more. Which is why it can be unreliable for commands that needs to be responsive.

Switching from one to another should be made much easier once #86 is merged in.

@gentlementlegen
Copy link
Member

/start

Copy link

Warning

You have reached your max task limit. Please close out some tasks before assigning new ones.

@gentlementlegen gentlementlegen self-assigned this Nov 22, 2024
Copy link

@gentlementlegen the deadline is at Fri, Nov 22, 8:46 AM UTC

Copy link

+ Evaluating results. Please wait...

Copy link

 [ 208.525 WXDAI ] 

@gentlementlegen
Contributions Overview
ViewContributionCountReward
IssueTask1200
IssueComment18.525
ReviewComment30
Conversation Incentives
CommentFormattingRelevancePriorityReward
@Keyrxng While I do agree that it will elongate the run time, I …
12.03
content:
  content:
    p:
      score: 0
      elementCount: 3
    a:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 149
  wordValue: 0.1
  result: 7.03
0.548.525
Resolves #28Depends on #86 QA: https://github.com/Meniole/co…
8
content:
  content:
    p:
      score: 0
      elementCount: 7
    h2:
      score: 1
      elementCount: 1
    ul:
      score: 0
      elementCount: 1
    li:
      score: 0.5
      elementCount: 4
    a:
      score: 5
      elementCount: 1
  result: 8
regex:
  wordCount: 74
  wordValue: 0
  result: 0
0.940
@whilefoo Could you please also have a look?
0
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 8
  wordValue: 0
  result: 0
0.240
@0x4007 It is hard to do a clear QA because it depends on a pull…
5
content:
  content:
    p:
      score: 0
      elementCount: 1
    a:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 38
  wordValue: 0
  result: 0
0.540

 [ 0.49 WXDAI ] 

@ishowvel
Contributions Overview
ViewContributionCountReward
IssueComment10.49
Conversation Incentives
CommentFormattingRelevancePriorityReward
Is this task up for grabs or is @jordan-ae still working on it
0.94
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 14
  wordValue: 0.1
  result: 0.94
0.540.49

 [ 0.02 WXDAI ] 

@jordan-ae
Contributions Overview
ViewContributionCountReward
IssueComment10.02
Conversation Incentives
CommentFormattingRelevancePriorityReward
Still working on it trying to get test pass. Giving me a bit of …
1.06
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 16
  wordValue: 0.1
  result: 1.06
040.02

 [ 3.142 WXDAI ] 

@Keyrxng
Contributions Overview
ViewContributionCountReward
IssueComment13.142
Conversation Incentives
CommentFormattingRelevancePriorityReward
This has a big chance of not performing well as a worker with th…
3.69
content:
  content:
    p:
      score: 0
      elementCount: 4
    ul:
      score: 0
      elementCount: 1
    li:
      score: 0.5
      elementCount: 2
  result: 1
regex:
  wordCount: 48
  wordValue: 0.1
  result: 2.69
0.843.142

 [ 183 WXDAI ] 

@whilefoo
Contributions Overview
ViewContributionCountReward
IssueSpecification1183
Conversation Incentives
CommentFormattingRelevancePriorityReward
The original implementation allowed for productive contributors …
15.25
content:
  content:
    p:
      score: 0
      elementCount: 11
    em:
      score: 0
      elementCount: 1
    a:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 232
  wordValue: 0.1
  result: 10.25
14183

 [ 7.392 WXDAI ] 

@0x4007
Contributions Overview
ViewContributionCountReward
ReviewComment47.392
Conversation Incentives
CommentFormattingRelevancePriorityReward
Code looks fine I just wish the QA was more clear.
0.77
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 11
  wordValue: 0.1
  result: 0.77
0.240.616
This description isn't very clear. Given the specification, I gu…
2
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 34
  wordValue: 0.1
  result: 2
0.846.4
Your QA isn't clear can you list the three scenarios and their o…
0.94
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 14
  wordValue: 0.1
  result: 0.94
0.140.376
Then let's test in prod
0.46
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 6
  wordValue: 0.1
  result: 0.46
040

@gentlementlegen
Copy link
Member

@0x4007 Maybe we should remove the x3 multiplier on the spec?

@0x4007
Copy link
Member

0x4007 commented Nov 27, 2024

I am not sure because it's hard to compensate for research required to author good specifications but 1. We should still limit to the assignee reward amount and 2. I wish that I received the credit since I'm the original author!

I think generally the order from highest to lowest for major contributions to completing projects should be

  1. The assignee (the contributor actually doing the work)
  2. The reviewers
  3. The specification author
  4. The rest

@gentlementlegen
Copy link
Member

@0x4007 There was a spec for splitting with the original post: ubiquity-os-marketplace/text-conversation-rewards#74 so I could have been split.

@rndquu rndquu removed this from Development Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment