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

feat!: symbol coefficients and configurable scores per group #68

Conversation

gentlementlegen
Copy link
Member

@gentlementlegen gentlementlegen commented Jul 23, 2024

Resolves #62
Depends on #58, #55

@gentlementlegen
Copy link
Member Author

/query @gentlementlegen

Copy link

ubiquibot-dev bot commented Jul 24, 2024

Property Value
Wallet 0x0fC1b909ba9265A846b82CF4CE352fc3e7EeB2ED

Copy link

ubiquibot bot commented Jul 24, 2024

Property Value
Wallet 0x0fC1b909ba9265A846b82CF4CE352fc3e7EeB2ED

@gentlementlegen
Copy link
Member Author

gentlementlegen commented Jul 24, 2024

What's new

The following changes in the configuration have been made:

  • the scores for tags are configurable per each select
  • regex can be used for the content count
  • at least one regex has to be provided (defaults to "\\b\\w+\\b")
  • routes for tests have been mocked properly for reward splitting (it was using the real data from the API)

QA run

Here is a test with two different regex counters.
Test run

@gentlementlegen gentlementlegen changed the title feat: symbol coefficients and configurable scores per group feat!: symbol coefficients and configurable scores per group Jul 24, 2024
@gentlementlegen gentlementlegen changed the base branch from development to main July 26, 2024 04:35
@gentlementlegen gentlementlegen changed the base branch from main to development July 26, 2024 04:35
@gentlementlegen gentlementlegen marked this pull request as ready for review July 26, 2024 04:36
README.md Outdated Show resolved Hide resolved
src/parser/data-purge-module.ts Outdated Show resolved Hide resolved
src/configuration/formatting-evaluator-config.ts Outdated Show resolved Hide resolved
src/configuration/formatting-evaluator-config.ts Outdated Show resolved Hide resolved
src/parser/github-comment-module.ts Outdated Show resolved Hide resolved
@rndquu
Copy link
Member

rndquu commented Jul 26, 2024

@gentlementlegen Check this config and this CI run which outputs:

Invalid incentives configuration detected, will revert to defaults.
{
  type: 62,
  schema: { anyOf: [ [Object], [Object] ], [Symbol(TypeBox.Kind)]: 'Union' },
  path: '/incentives/formattingEvaluator',
  value: {
    scores: {
      br: 0,
      code: 1,
      p: 1,
      em: 0,
      img: 0,
      strong: 0,
      blockquote: 0,
      h1: 1,
      h2: 1,
      h3: 1,
      h4: 1,
      h5: 1,
      h6: 1,
      a: 1,
      li: 1,
      td: 1,
      hr: 0
    },
    multipliers: [
      [Object], [Object],
      [Object], [Object],
      [Object], [Object],
      [Object], [Object],
      [Object], [Object]
    ]
  },
  message: 'Expected union value'
}

Is this error expected?

src/configuration/config-reader.ts Outdated Show resolved Hide resolved
src/configuration/incentives.ts Outdated Show resolved Hide resolved
src/parser/formatting-evaluator-module.ts Outdated Show resolved Hide resolved
src/parser/github-comment-module.ts Show resolved Hide resolved
src/parser/formatting-evaluator-module.ts Show resolved Hide resolved
Comment on lines 61 to 68
? Object.values(formatting).reduce((acc, curr) => {
let sum = new Decimal(0);
for (const symbol of Object.keys(curr.symbols)) {
sum = sum.add(
new Decimal(curr.symbols[symbol].count)
.mul(curr.symbols[symbol].multiplier)
.mul(multiplierFactor.formattingMultiplier)
.mul(curr.count)
.mul(multiplierFactor.wordValue)
),
new Decimal(0)
)
.mul(curr.score)
Copy link
Contributor

@whilefoo whilefoo Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my memory score was basically a reward for each tag, for example if there are 5 p tags and the score in the config is set to 5, then the reward is 25.

If I understand correctly now symbols/regex is counted inside the tag and multiplied by the multiplier for the symbol and then multiplied by formatting multiplier which is a multiplier that is same for all symbols and tags and then multiplied by score which is like a multiplier that is same for all symbols inside the tag?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whilefoo yes that's pretty much what is happening there. Shall it be changed? If so I can take care of it in a separate PR.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@gentlementlegen gentlementlegen changed the base branch from development to main August 15, 2024 05:52
@gentlementlegen gentlementlegen changed the base branch from main to development August 15, 2024 05:52
@gentlementlegen gentlementlegen marked this pull request as ready for review August 15, 2024 09:09
@Keyrxng
Copy link
Contributor

Keyrxng commented Aug 15, 2024

@gentlementlegen where can I find a working config setup? Is the readme one up to date? I'm getting typebox errors with it the same as rndquu had

I think we should eslint.ignore README.md and have the correct indentation etc copy and pasted from a working config in all plugin repos starting from plugins: as it has 0 indentation.

Invalid incentives configuration detected, will revert to defaults.
{
  type: 62,
  schema: {
    default: null,
    anyOf: [ [Object], [Object] ],
    [Symbol(TypeBox.Kind)]: 'Union'
  },
  path: '/incentives/formattingEvaluator',
  value: {
    multipliers: [
      [Object], [Object],
      [Object], [Object],
      [Object], [Object],
      [Object], [Object],
      [Object], [Object]
    ]
  },
  message: 'Expected union value'
}

The union type is the roles but it seems like the defaults are also throwing looking at this log. I have tried two of the configs from this thread and the one from the readme. I have tried making small adjustments like indentation and using quotes for strings etc but no luck. The runs seems to be exiting without error but fails to load any modules.

I'm not 100% on my triage but in my case all the user sees on the issue is + Evaluating results. Please wait... and has no feedback without manually reviewing the logs. Is it something that I am doing wrong?


PERMIT_FEE_RATE=""
PERMIT_TREASURY_GITHUB_USERNAME=""
PERMIT_ERC20_TOKENS_NO_FEE_WHITELIST=""

These are optional repo secrets right?

and the only required ones are

OPENAI_API_KEY
SUPABASE_KEY
SUPABASE_URL
X25519_PRIVATE_KEY

I did get feedback when I had missing secrets which is good

@gentlementlegen
Copy link
Member Author

@Keyrxng There were major configuration changes after the last review requested changes, so here is one that should work for you:

  - uses:
    - plugin: Meniole/conversation-rewards
      with:
        evmNetworkId: 100
        evmPrivateEncrypted: "some-key"
        incentives:
          contentEvaluator: {}
          userExtractor:
            redeemTask: true
          dataPurge: {}
          formattingEvaluator:
            multipliers:
              - select: [ ISSUE_SPECIFICATION ]
                multiplier: 1
                rewards:
                  regex:
                    "\\b\\w+\\b": 0.1
                    "\\b\\W+\\b": 0.1
                  html:
                    br: 0
                    code: 1
                    p: 1
                    em: 0
                    img: 0
                    strong: 0
                    blockquote: 0
                    h1: 1
                    h2: 1
                    h3: 1
                    h4: 1
                    h5: 1
                    h6: 1
                    a: 1
                    li: 1
                    td: 1
                    hr: 0
              - select: [ ISSUE_AUTHOR ]
                multiplier: 1
                rewards:
                  regex:
                    "\\b\\w+\\b": 0.2
              - select: [ ISSUE_ASSIGNEE ]
                multiplier: 0
                rewards:
                  regex:
                    "\\b\\w+\\b": 0
              - select: [ ISSUE_COLLABORATOR ]
                multiplier: 1
                rewards:
                  regex:
                    "\\b\\w+\\b": 0.1
              - select: [ ISSUE_CONTRIBUTOR ]
                multiplier: 0.25
                rewards:
                  regex:
                    "\\b\\w+\\b": 0.1
              - select: [ PULL_SPECIFICATION ]
                multiplier: 0
                rewards:
                  regex:
                    "\\b\\w+\\b": 0
              - select: [ PULL_AUTHOR ]
                multiplier: 2
                rewards:
                  regex:
                  "\\b\\w+\\b": 0.2
              - select: [ PULL_ASSIGNEE ]
                multiplier: 1
                rewards:
                  regex:
                    "\\b\\w+\\b": 0.1
              - select: [ PULL_COLLABORATOR ]
                multiplier: 1
                rewards:
                  regex:
                    "\\b\\w+\\b": 0.1
              - select: [ PULL_CONTRIBUTOR ]
                multiplier: 0.25
                rewards:
                  regex:
                    "\\b\\w+\\b": 0.1
            permitGeneration: {}
            githubComment:
              post: true
              debug: true

The problem about the union type is actually that the error is within the incentives configuration but the message is misleading. I don't think I can fix it because it is within the library itself, not in my code. The README should be up-to-date as well, let me know.

const formattingTotal = formatting
? Object.values(formatting).reduce((acc, curr) => {
let sum = new Decimal(0);
for (const symbol of Object.keys(curr.symbols)) {
sum = sum.add(
new Decimal(curr.symbols[symbol].count)
.mul(curr.symbols[symbol].multiplier)
.mul(multiplierFactor.formattingMultiplier)
.mul(multiplierFactor.multiplier)
.mul(curr.score)
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you help me grasp how this calculation work?

At first for every tag (<a>), the number of regex matches is counted (each regex has its own multiplier) and has a so called score which is defined in the config.

Then for the calculation, for every regex (symbol) the count is multiplied by the regex multiplier which makes sense, then that is multiplied by a "general" multiplier and multiplied by a score which is a multiplier for the tag I assume.

I had to read the code a couple of times and it's hard to keep track of all different names for same things, like symbols = regex, score = multiplier for a tag and variables names are confusing such as wordCount for regex count

Copy link
Member

@0x4007 0x4007 Aug 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least in the config we renamed things accordingly. For example I'm pretty sure we renamed symbol to regex, so we should do the same in the code here.

Also score should just sum per instance. For example if we want to award $1 for every paragraph, then score for p should be 1.

score is not supposed to be a multiplier.

However in the config I believe we also support a multiplier property. This should be separate from score.

Copy link
Member Author

@gentlementlegen gentlementlegen Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will rename the variables to match the configuration. The score concept does not exist (yet) in v2 and can be added in a separate PR. Only multipliers exist for now. Created #83

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also score should just sum per instance. For example if we want to award $1 for every paragraph, then score for p should be 1.

score is not supposed to be a multiplier.

that was also my understanding but currently in the code it's a multiplier

@Keyrxng
Copy link
Contributor

Keyrxng commented Aug 21, 2024

I was reading the wrong step in the run but things appear to be the same for me as before. Using the config you gave me and little feedback on the issue.

Am I the only one that can't run this? @whilefoo could you?

Either I'm stupid af or it's going to be as tricky for partner's to get right as it appears to be for me lol.

Would it be possible to use T.Enum instead of T.Union considering it's inside a T.Array would that work? It might resolve the typebox union error that's happening. I don't think it's a good idea to allow that to remain unresolved. If it prevents errors down the line from being logged especially.

@gentlementlegen
Copy link
Member Author

@Keyrxng can make it as another task because I think it is tricky to solve. The Union is needed to allow for null or a defined object with the configuration. the problem seems to be that the logged error is wrong. It is weird however that you encounter so much trouble running it.

@Keyrxng
Copy link
Contributor

Keyrxng commented Aug 22, 2024

It is weird however that you encounter so much trouble running it.

Very weird you'd expect that a fork of your branch, an exact config match and up to date kernel that it would work out-of-the-box.

For sure another issue to address the typebox issue and improve feedback on the issue for scenarios like the one I've been experiencing.


I also have a concern regarding env vars in this repo.

It seems that env and pluginSettings are out-of-sync. In the new config we pass the privateKey via the org config not repo secrets.

Partner's that want to use this plugin need to use their org config to pass env vars as we don't want them to have to fork this repo. I think permit_fee etc is set by us so that makes sense to keep but I'm unsure about the rest but basically can this be cleaned up to what is actually passed via secrets? In another PR if or whatever you think is best

@gentlementlegen
Copy link
Member Author

@Keyrxng please make a spec 😄

@gentlementlegen gentlementlegen merged commit dae0b4d into ubiquity-os-marketplace:development Aug 26, 2024
3 checks passed
@gentlementlegen gentlementlegen deleted the feat/symbol-coefficient branch August 26, 2024 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add symbol coefficient and make html tags configurable per target group
5 participants