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.

Sign up for GitHub

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
Open

Added "defaultMemoryMbytes" to actor.json #55

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
363b873
Added "defaultMemoryMbytes" to actor.json
jancurn Dec 5, 2024
d71cade
Added comments
jancurn Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions pages/ACTOR_FILE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ The file has the following structure:
"minMemoryMbytes": 128,
"maxMemoryMbytes": 4096,

// 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",
Comment on lines +36 to +39
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.


// Links to other Actor defintion files
"dockerfile": "./Dockerfile", // If omitted, the system looks for "./Dockerfile" and "../Dockerfile"
"readme": "./README.md", // If omitted, the system looks for "./ACTOR.md" and "../README.md"
Expand Down
Loading