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

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

Closed
gentlementlegen opened this issue Jul 22, 2024 · 22 comments · Fixed by #21
Closed

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

gentlementlegen opened this issue Jul 22, 2024 · 22 comments · Fixed by #21

Comments

@gentlementlegen
Copy link
Member

gentlementlegen commented Jul 22, 2024

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:

  • display a warning message when the task is above the configuration threshold
  • do not display the warning if it is under that threshold
  • change the configuration to accept a string representing a duration instead of a time stamp
  • related tests
@gentlementlegen
Copy link
Member Author

@Keyrxng would this be the fix for it? ubiquity/work.ubq.fi#74 (comment)

@Keyrxng
Copy link
Member

Keyrxng commented Jul 23, 2024

yeah @gentlementlegen aae3cba resolved here in #10

@gentlementlegen
Copy link
Member Author

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

@0x4007
Copy link
Member

0x4007 commented Jul 23, 2024

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

@gentlementlegen
Copy link
Member Author

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
Copy link
Member

Keyrxng commented Jul 23, 2024

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

ubq-testing#5 (comment)

image

@0x4007
Copy link
Member

0x4007 commented Jul 23, 2024

Use <code> not <samp>

image

image

@gentlementlegen
Copy link
Member Author

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

@Keyrxng
Copy link
Member

Keyrxng commented Jul 23, 2024

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
Copy link
Member

0x4007 commented Jul 24, 2024

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
Copy link
Member Author

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

@Keyrxng
Copy link
Member

Keyrxng commented Jul 24, 2024

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
Copy link
Member Author

@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.

@Keyrxng
Copy link
Member

Keyrxng commented Aug 13, 2024

@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
Copy link
Member Author

@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
Copy link
Member

Keyrxng commented Aug 13, 2024

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

Copy link

ubiquibot bot commented Aug 13, 2024

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

Copy link
Contributor

ubiquity-os bot commented Aug 24, 2024

[ 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
Copy link
Member

0x4007 commented Aug 25, 2024

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
Copy link
Member Author

gentlementlegen commented Aug 25, 2024

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
Copy link
Member

0x4007 commented Aug 25, 2024

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
Copy link
Member Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants