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

Added "defaultMemoryMbytes" to actor.json #55

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jancurn
Copy link
Member

@jancurn jancurn commented Dec 5, 2024

@jancurn jancurn requested a review from fnesveda December 5, 2024 22:08
Comment on lines +36 to +39
// Used when user doesn't specify memory, the 𝔸ctor run will start with this amount.
// It might be a basic arithemtic expression referencing ${variables} from Actor input.
// This right value can optimize user experience vs. compute costs.
"defaultMemoryMbytes": "${maxCrawlPages} * 256 + 128",
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we want to have it this customizable? Right now the memory has to be a power of 2, which would be hard to enforce with a custom expression. We could relax the "power of 2" requirement, but I'm a bit worried about the implications on job allocation to workers if we allow completely custom memory.

Also it might be more readable to put defaultMemoryMbytes between minMemoryMbytes and maxMemoryMbytes, to make it a bit more clear that the calculated value is clamped from both sides, and have one comment for all three together.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now the memory has to be a power of 2,

We could say we always ceil it to the next higher value available by the platform. This should apply to all three parameters.

Are we sure we want to have it this customizable?

I think it's better to use an expression rather than some A and B linear coefficients.

Copy link
Member

@metalwarrior665 metalwarrior665 left a comment

Choose a reason for hiding this comment

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

I like this. This would probably remove the need to have charge the actor-start per GB or maybe charge it at all. No, this is just a default, they could override it so we would still have to charge per GB for the wrong configurers

@jancurn
Copy link
Member Author

jancurn commented Dec 6, 2024

I like this. This would probably remove the need to have charge the actor-start per GB or maybe charge it at all. No, this is just a default, they could override it so we would still have to charge per GB for the wrong configurers

In theory in the future we might disable ability to select custom memory, or per developer setting. This opens the path.

@jancurn
Copy link
Member Author

jancurn commented Dec 6, 2024

BTW perhaps this would be cleaner:

memory: {
  minMbytes: 128,
  maxMbytes: 4096,
  defaultMbytes: "${maxCrawlPages} * 256 + 128"
  // default: 1024 - this also works
}

What do you think guys?

@fnesveda
Copy link
Member

fnesveda commented Dec 6, 2024

How about this?

  • move mbytes suffix one higher
    • it's cleaner, but if there would be some other memory config which isn't directly related to the mbytes of memory available, it can't go into the object
  • prefix the variables with input. to make it clear that they're referring to input values, not other values from actor.json
memoryMbytes: {
    min: 128,
    default: "${input.maxCrawlPages} * 256 + 128"
    max: 4096,
}

@jancurn
Copy link
Member Author

jancurn commented Dec 6, 2024

IMO it's better to keep just memory, to have flexibility to add more stuff in the future. Also the convention is that variable names must end with the unit, not parent objects. So:

memory: {
    minMbytes: 128,
    defaultMbytes: "${input.maxCrawlPages} * 256 + 128"
    maxMbytes: 4096,
}

@jancurn
Copy link
Member Author

jancurn commented Dec 6, 2024

Or maybe we could make it even more generic and future proof:

runtime: {
    memoryMinMbytes: 128,
    memoryDefaultMbytes: "${input.maxCrawlPages} * 256 + 128"
    memoryMaxMbytes: 4096,
}

Plus I agree with ${input.maxCrawlPages}, it just needs to be consistent syntax with OUTPUT_SCHEMA.json

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.

3 participants