ubiquibot / command-start-stop

Manager user commands to assign / un-assign themselves to tasks.
0 stars 10 forks source link

Do not show warning message on tasks that were not created a long time ago #14

Closed gentlementlegen closed 3 weeks ago

gentlementlegen commented 1 month ago

The bot shows a warning message on tasks that were opened recently, as seen here. It should only display this warning above a certain threshold, which comes from the configuration.

A possible cause would be that value missing in the current configuration. If that is the case, the default threshold should be set, and definitely above 0 days.


Tasks to be carried out:

gentlementlegen commented 1 month ago

@Keyrxng would this be the fix for it? https://github.com/ubiquity/work.ubq.fi/issues/74#issuecomment-2242566257

Keyrxng commented 1 month ago

yeah @gentlementlegen https://github.com/ubiquibot/command-start-stop/pull/10/commits/aae3cbac583a433b6c0e52986f462a9546520041 resolved here in #10

gentlementlegen commented 1 month ago

Ok cool thank you, I will close this one after we got your fix merged.

0x4007 commented 1 month ago

So the font is wrong be sure to match the styles of the old assign message exactly.

gentlementlegen commented 1 month ago

I triple checked I cannot see any font difference.

image image image image

Maybe there is a difference on mobile devices if you are using one?

Keyrxng commented 1 month ago

registered wallet seems to wrap only when the warning is visible for some reason, nothing springs to mind on how to fix that

https://github.com/ubq-testing/command-start-stop/issues/5#issuecomment-2243850760

image

0x4007 commented 1 month ago

Use <code> not <samp>

image

image

gentlementlegen commented 1 month ago

@0x4007 Thanks for the screenshot, it seems to be more specific to mobile for the font. Maybe a font fallback happening there.

Keyrxng commented 1 month ago

Been reading through the GFM and CommonMark specs, GH docs etc but haven't been able to find any other workarounds other than <samp>. We'll just have to live with the whitespace if using <code> blocks so which of the below options is best, included <samp> for easy comparison. I can't find any other workarounds with these tags or others such as <kbd> etc.


1

Warning!This task was created over 10 days ago. Please confirm that this issue specification is accurate before starting.
DeadlineMon, Jul 22, 10:25 PM UTC
Registered WalletRegister your wallet address using the following slash command: `/wallet 0x0000...0000`


2

Warning!This task was created over 10 days ago. Please confirm that this issue specification is accurate before starting.
DeadlineMon, Jul 22, 10:25 PM UTC
Registered WalletRegister your wallet address using the following slash command: `/wallet 0x0000...0000`


3

Warning!This task was created over 10 days ago. Please confirm that this issue specification is accurate before starting.
DeadlineMon, Jul 22, 10:25 PM UTC
Registered WalletRegister your wallet address using the following slash command: `/wallet 0x0000...0000`

0x4007 commented 1 month ago

Been reading through the GFM and CommonMark specs, GH docs etc but haven't been able to find any other workarounds other than <samp>. We'll just have to live with the whitespace if using <code> blocks so which of the below options is best, included <samp> for easy comparison. I can't find any other workarounds with these tags or others such as <kbd> etc.

If its only a problem on mobile then perhaps <samp> is the best decision!

gentlementlegen commented 1 month ago

Number 3 seems to be the best display in your propositions, if that works on mobile then let's go with it.

Keyrxng commented 1 month ago

If its only a problem on mobile then perhaps is the best decision!

Yeah it's only mobile that <samp> seems to have slightly different rendering

Number 3 seems to be the best display in your propositions, if that works on mobile then let's go with it.

I agree it looks best but no it is somewhat different although personally I think <samp> is the way to go because it'll most commonly be seen via desktop vs mobile and I'd rather no whitespace than to accommodate the lesser used platform.

Sounds like option 3 is preferred and that's how the code is right now so no changes needed in this regard

gentlementlegen commented 1 month ago

@Keyrxng You mentioned that the bug was fixed, but now the opposite might be happening, see message here: https://github.com/ubiquity/pay.ubq.fi/issues/237#issuecomment-2285326567

I also suggest that we change the configuration format to take a string representing a duration and not a number which is hard to understand and error prone.

Keyrxng commented 1 month ago

@Keyrxng You mentioned that the bug was fixed, but now the opposite might be happening, see message here: ubiquity/pay.ubq.fi#237 (comment)

I also suggest that we change the configuration format to take a string representing a duration and not a number which is hard to understand and error prone.

@gentlementlegen you want to add it into the spec/title here and I'll handle both?

gentlementlegen commented 1 month ago

@Keyrxng Done. Also, I guess the font issue should be carried out in a different issue most likely, if there is not once created already.

Keyrxng commented 1 month ago

Also, I guess the font issue should be carried out in a different issue most likely, if there is not once created already.

I'm certain that we landed on sticking with <samp> and supporting the most used platform as it's only an issue on mobile so no need to do anything with that

ubiquibot[bot] commented 1 month ago

@Keyrxng the deadline is at 2024-08-13T08:53:45.783Z

ubiquity-os[bot] commented 3 weeks ago

[ 50 WXDAI ]

@Keyrxng
Contributions Overview
View Contribution Count Reward
Issue Task 1 50
Issue Comment 6 0
Review Comment 5 0
Conversation Incentives
Comment Formatting Relevance Reward
yeah @gentlementlegen https://github.com/ubiquibot/command-star…
0
content:
  p:
    count: 7
    score: 1
wordValue: 0
formattingMultiplier: 0
0.1 -
`registered wallet` seems to wrap only when the warning …
0
content:
  p:
    count: 24
    score: 1
  code:
    count: 2
    score: 1
  img:
    count: 1
    score: 0
wordValue: 0
formattingMultiplier: 0
0.2 -
Been reading through the GFM and CommonMark specs, GH docs etc b…
0
content:
  h2:
    count: 173
    score: 1
  code:
    count: 10
    score: 1
  h3:
    count: 3
    score: 1
wordValue: 0
formattingMultiplier: 0
0.5 -
Yeah it's only mobile that `<samp>` seems to have …
0
content:
  p:
    count: 76
    score: 1
  code:
    count: 2
    score: 1
wordValue: 0
formattingMultiplier: 0
0.4 -
@gentlementlegen you want to add it into the spec/title here and…
0
content:
  p:
    count: 14
    score: 1
wordValue: 0
formattingMultiplier: 0
0.1 -
I'm certain that we landed on sticking with `<samp> …
0
content:
  p:
    count: 30
    score: 1
  code:
    count: 1
    score: 1
wordValue: 0
formattingMultiplier: 0
0.2 -
Resolves #14 I'm assuming this will be merged quickly and once …
0
content:
  p:
    count: 47
    score: 1
  ul:
    count: 32
    score: 0
  li:
    count: 32
    score: 1
  code:
    count: 20
    score: 1
  pre:
    count: 16
    score: 0
  a:
    count: 10
    score: 1
wordValue: 0
formattingMultiplier: 0
0.8 -
Oop yeah not intended, forgot to run format after deciding to ad…
0
content:
  p:
    count: 15
    score: 1
wordValue: 0.2
formattingMultiplier: 0
1 -
- `reviewDelayTolerance`: is used when there is no revie…
0
content:
  ul:
    count: 37
    score: 0
  li:
    count: 37
    score: 1
  code:
    count: 6
    score: 1
wordValue: 0.2
formattingMultiplier: 0
1 -
@0x4007 `Month(s)` is not a supported unit with ms and t…
0
content:
  p:
    count: 17
    score: 1
  code:
    count: 1
    score: 1
wordValue: 0.2
formattingMultiplier: 0
1 -
Bumping this for review as it also now resolves the issue expose…
0
content:
  p:
    count: 23
    score: 1
  code:
    count: 1
    score: 1
wordValue: 0.2
formattingMultiplier: 0
1 -

[ 40.955 WXDAI ]

@gentlementlegen
Contributions Overview
View Contribution Count Reward
Issue Specification 1 31.2
Issue Comment 7 8.38
Review Comment 4 1.375
Conversation Incentives
Comment Formatting Relevance Reward
The bot shows a warning message on tasks that were opened recent…
31.2
content:
  h2:
    count: 58
    score: 1
  a:
    count: 1
    score: 1
  p:
    count: 5
    score: 1
  ul:
    count: 40
    score: 0
  li:
    count: 40
    score: 1
wordValue: 0.1
formattingMultiplier: 3
1 31.2
@Keyrxng would this be the fix for it? https://github.com/ubiqui…
1.8
content:
  p:
    count: 9
    score: 1
wordValue: 0.2
formattingMultiplier: 1
0.2 0.36
Ok cool thank you, I will close this one after we got your fix m…
3
content:
  p:
    count: 15
    score: 1
wordValue: 0.2
formattingMultiplier: 1
- -
I triple checked I cannot see any font difference. <img widt…
7.6
content:
  p:
    count: 38
    score: 1
wordValue: 0.2
formattingMultiplier: 1
- -
@0x4007 Thanks for the screenshot, it seems to be more specific …
4.4
content:
  p:
    count: 22
    score: 1
wordValue: 0.2
formattingMultiplier: 1
- -
Number 3 seems to be the best display in your propositions, if t…
4.2
content:
  p:
    count: 21
    score: 1
wordValue: 0.2
formattingMultiplier: 1
- -
@Keyrxng You mentioned that the bug was fixed, but now the oppos…
9.4
content:
  p:
    count: 47
    score: 1
wordValue: 0.2
formattingMultiplier: 1
0.8 7.52
@Keyrxng Done. Also, I guess the font issue should be carried ou…
5
content:
  p:
    count: 25
    score: 1
wordValue: 0.2
formattingMultiplier: 1
0.1 0.5
Might wanna change `(1000 * 60 * 60)` tu use `ms`
0.425
content:
  p:
    count: 11
    score: 1
  code:
    count: 6
    score: 1
wordValue: 0.1
formattingMultiplier: 0.25
1 0.425
Maybe this check is redundant since you return `false` w…
0.425
content:
  p:
    count: 15
    score: 1
  code:
    count: 2
    score: 1
wordValue: 0.1
formattingMultiplier: 0.25
1 0.425
@Keyrxng I believe this is not intended?
0.175
content:
  p:
    count: 7
    score: 1
wordValue: 0.1
formattingMultiplier: 0.25
1 0.175
@Keyrxng Can you please fix this? I am fine with the PR otherwis…
0.35
content:
  p:
    count: 14
    score: 1
wordValue: 0.1
formattingMultiplier: 0.25
1 0.35

[ 7.4 WXDAI ]

@0x4007
Contributions Overview
View Contribution Count Reward
Issue Comment 3 0
Review Comment 4 7.4
Conversation Incentives
Comment Formatting Relevance Reward
So the font is wrong be sure to match the styles of the old assi…
1.7
content:
  p:
    count: 17
    score: 1
wordValue: 0.1
formattingMultiplier: 1
- -
Use `<code>` not `<samp>` ![image](h…
0.6
content:
  p:
    count: 4
    score: 1
  code:
    count: 2
    score: 1
  img:
    count: 2
    score: 0
wordValue: 0.1
formattingMultiplier: 1
- -
If its only a problem on mobile then perhaps `<samp>&#…
1.5
content:
  p:
    count: 14
    score: 1
  code:
    count: 1
    score: 1
wordValue: 0.1
formattingMultiplier: 1
- -
I am confused what exactly is `reviewDelayTolerance`
0.8
content:
  p:
    count: 7
    score: 1
  code:
    count: 1
    score: 1
wordValue: 0.1
formattingMultiplier: 1
1 0.8
```suggestion reviewDelayTolerance: "1 Day" …
1.4
content:
  pre:
    count: 6
    score: 0
  code:
    count: 6
    score: 1
  p:
    count: 8
    score: 1
wordValue: 0.1
formattingMultiplier: 1
1 1.4
```suggestion reviewDelayTolerance: T.String({ …
1.2
content:
  pre:
    count: 12
    score: 0
  code:
    count: 12
    score: 1
wordValue: 0.1
formattingMultiplier: 1
1 1.2
Warning message for a task being old and they should confirm the…
4
content:
  p:
    count: 40
    score: 1
wordValue: 0.1
formattingMultiplier: 1
1 4

[ 1.15 WXDAI ]

@whilefoo
Contributions Overview
View Contribution Count Reward
Review Comment 3 1.15
Conversation Incentives
Comment Formatting Relevance Reward
you can leave this default as `{}`
0.2
content:
  p:
    count: 7
    score: 1
  code:
    count: 1
    score: 1
wordValue: 0.1
formattingMultiplier: 0.25
1 0.2
is there a reason to convert to days? just get milliseconds of b…
0.55
content:
  p:
    count: 21
    score: 1
  code:
    count: 1
    score: 1
wordValue: 0.1
formattingMultiplier: 0.25
1 0.55
why is it converted to hours if `getTimeValue(reviewDelayTol…
0.4
content:
  p:
    count: 15
    score: 1
  code:
    count: 1
    score: 1
wordValue: 0.1
formattingMultiplier: 0.25
1 0.4
0x4007 commented 3 weeks ago

I think we need to tweak the qualitative analysis. Somehow I got 0 relevance on my comments which didn't seem to be the case before with gpt3.5 10x samples.

Also I should be getting img credit.

Seems like there's problems with quantitative analysis as well

@gentlementlegen

gentlementlegen commented 3 weeks ago

For the img it is currently set to 0 so you can just change the configuration. For the relevance I will read the logs more in detail.


After reading the logs, it is possible that the allocated max_tokens fell short with the dummy test leading to empty relevances here. I guess we could add a margin (say for example 10 extra tokens) to make sure there is a result.

0x4007 commented 3 weeks ago

Margin seems imprecise, which leads me to believe that the approach is wrong. Perhaps the approach should be reevaluated so that the math is perfect.

gentlementlegen commented 3 weeks ago

@0x4007 It was supposed to be because the length of results is predictable, but when allocating only one token the result seems to be {} without the content (should be { "1234" : 0.5 }. I'll investigate.