feat: add calculate-memory command to actor #980
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| import { join, resolve } from 'node:path'; | ||
| import process from 'node:process'; | ||
|
|
||
| import { calculateRunDynamicMemory } from '@apify/actor-memory-expression'; | ||
|
|
||
| import { ApifyCommand } from '../../lib/command-framework/apify-command.js'; | ||
| import { Flags } from '../../lib/command-framework/flags.js'; | ||
| import { CommandExitCodes } from '../../lib/consts.js'; | ||
| import { useActorConfig } from '../../lib/hooks/useActorConfig.js'; | ||
| import { error, info, success } from '../../lib/outputs.js'; | ||
| import { getJsonFileContent, getLocalKeyValueStorePath } from '../../lib/utils.js'; | ||
|
|
||
| const DEFAULT_INPUT_PATH = join(getLocalKeyValueStorePath('default'), 'INPUT.json'); | ||
|
|
||
| /** | ||
| * This command can be used to test dynamic memory calculation expressions | ||
| * defined in actor.json or provided via command-line flag. | ||
| * | ||
| * Dynamic memory allows Actors to adjust their memory usage based on input data | ||
| * and run options, optimizing resource allocation and costs. | ||
| */ | ||
| export class ActorCalculateMemoryCommand extends ApifyCommand<typeof ActorCalculateMemoryCommand> { | ||
|
Contributor
There was a problem hiding this comment. What is this magic? Why is it basically extending itself? 😅
Contributor
Author
There was a problem hiding this comment. It's used for types of maybe @vladfrangu would add more info here
Member
There was a problem hiding this comment. It's not extending itself but rather referencing itself in the extension class -> allows us to infer the types of the flags and args for |
||
| static override name = 'calculate-memory' as const; | ||
|
|
||
| static override description = | ||
| `Calculates the Actor’s dynamic memory usage based on a memory expression from actor.json, input data, and run options.`; | ||
|
|
||
| /** | ||
| * Additional run options exist (e.g., memoryMbytes, disksMbytes, etc.), | ||
| * but we intentionally omit them here. These options are rarely needed and | ||
| * exposing them would introduce unnecessary confusion for users. | ||
| */ | ||
| static override flags = { | ||
| input: Flags.string({ | ||
| description: 'Path to the input JSON file used for the calculation.', | ||
| required: false, | ||
| default: DEFAULT_INPUT_PATH, | ||
| }), | ||
| defaultMemoryMbytes: Flags.string({ | ||
| description: | ||
| 'Memory-calculation expression (in MB). If omitted, the value is loaded from the actor.json file.', | ||
| required: false, | ||
| }), | ||
| build: Flags.string({ | ||
| description: 'Actor build version or build tag to evaluate the expression with.', | ||
| required: false, | ||
| }), | ||
| timeoutSecs: Flags.integer({ | ||
|
Contributor
There was a problem hiding this comment. I can imagine this being slightly confusing (because it is confusing to me 😅 ). Intuitively, why should The only slightly related fields from run options are The bottom line is that I find it really weird to have these top level params that have zero effect and are there simply because they are part of Actor run options. I thought about dropping the options altogether from the memory calculation (that'd be a bigger change) but I guess I just wonder, is it possible to provide these also as a JSON file / object? Which would be probably inconsistent with other CLI commands but would make more intuitive sense.
Contributor
Author
There was a problem hiding this comment. Makes sense, I was also thinking about that as well. I'd not rather provide options as JSON, probably not many devs would need it. The idea behind providing path to Regarding IMHO it's cleaner for me to provide options as flags from DX point of view, but if you feel like it would be better to provide them in additional file I can do that as well.
Contributor
There was a problem hiding this comment.
Totally agree here 👍
Remind me, what is
From DX, I'd argue that providing a param that doesn't do anything is at least a little weird 😄
Contributor
Author
There was a problem hiding this comment.
It's
Makes sense, I removed |
||
| description: 'Maximum run timeout, in seconds.', | ||
| required: false, | ||
| }), | ||
| maxItems: Flags.integer({ | ||
| description: 'Maximum number of items Actor can output.', | ||
| required: false, | ||
| }), | ||
| maxTotalChargeUsd: Flags.integer({ | ||
| description: 'Maximum total charge in USD.', | ||
| required: false, | ||
| }), | ||
| }; | ||
|
|
||
| async run() { | ||
| const { input, defaultMemoryMbytes, ...runOptions } = this.flags; | ||
|
|
||
| let memoryExpression: string | undefined = defaultMemoryMbytes; | ||
|
|
||
| // If not provided via flag, try to load from actor.json | ||
| if (!memoryExpression) { | ||
| memoryExpression = await this.getExpressionFromConfig(); | ||
| } | ||
|
|
||
| if (!memoryExpression) { | ||
| throw new Error( | ||
| `No memory-calculation expression found. Provide it via the --defaultMemoryMbytes flag or define defaultMemoryMbytes in actor.json.`, | ||
| ); | ||
| } | ||
|
|
||
| const inputPath = resolve(process.cwd(), this.flags.input); | ||
| const inputJson = getJsonFileContent(inputPath) ?? {}; | ||
|
|
||
| info({ message: `Evaluating memory expression: ${memoryExpression}` }); | ||
|
|
||
| try { | ||
| const result = await calculateRunDynamicMemory(memoryExpression, { | ||
| input: inputJson, | ||
| runOptions, | ||
| }); | ||
| success({ message: `Calculated memory: ${result} MB`, stdout: true }); | ||
| } catch (err) { | ||
| error({ message: `Memory calculation failed: ${(err as Error).message}` }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Helper to load the `defaultMemoryMbytes` expression from actor.json. | ||
| */ | ||
| private async getExpressionFromConfig(): Promise<string | undefined> { | ||
|
Contributor
There was a problem hiding this comment. (nit) There seems to be a lot of boilerplate and error handling for what seems to be a rather generic functionality: get Actor config 🤔 There is no existing utility?
Contributor
Author
There was a problem hiding this comment. Well we use one functionality
Member
There was a problem hiding this comment. This is the existing utility, and it handles multiple cases (invalid config file, non-existing file or existing file). I do think you could probably skip the exists == false check tho... It will just return undefined -> bubble up top |
||
| const cwd = process.cwd(); | ||
| const localConfigResult = await useActorConfig({ cwd }); | ||
|
|
||
| if (localConfigResult.isErr()) { | ||
| const { message, cause } = localConfigResult.unwrapErr(); | ||
|
|
||
| error({ message: `${message}${cause ? `\n ${cause.message}` : ''}` }); | ||
| process.exitCode = CommandExitCodes.InvalidActorJson; | ||
| return; | ||
| } | ||
|
|
||
| const { config: localConfig } = localConfigResult.unwrap(); | ||
| return localConfig?.defaultMemoryMbytes?.toString(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| import { mkdirSync, writeFileSync } from 'node:fs'; | ||
| import { mkdir } from 'node:fs/promises'; | ||
| import { dirname } from 'node:path'; | ||
|
|
||
| import { ActorCalculateMemoryCommand } from '../../../../src/commands/actor/calculate-memory.js'; | ||
| import { testRunCommand } from '../../../../src/lib/command-framework/apify-command.js'; | ||
| import { EMPTY_LOCAL_CONFIG, LOCAL_CONFIG_PATH } from '../../../../src/lib/consts.js'; | ||
| import { getLocalKeyValueStorePath } from '../../../../src/lib/utils.js'; | ||
| import { useConsoleSpy } from '../../../__setup__/hooks/useConsoleSpy.js'; | ||
| import { useTempPath } from '../../../__setup__/hooks/useTempPath.js'; | ||
| import { resetCwdCaches } from '../../../__setup__/reset-cwd-caches.js'; | ||
|
|
||
| const { beforeAllCalls, afterAllCalls, joinPath } = useTempPath('calculate-memory', { | ||
| create: true, | ||
| remove: true, | ||
| cwd: true, | ||
| cwdParent: false, | ||
| }); | ||
|
|
||
| const { lastErrorMessage, lastLogMessage } = useConsoleSpy(); | ||
|
|
||
| const createActorJson = async (overrides: Record<string, unknown> = {}) => { | ||
| const actorJson = { ...EMPTY_LOCAL_CONFIG, ...overrides }; | ||
|
|
||
| await mkdir(joinPath('.actor'), { recursive: true }); | ||
| writeFileSync(joinPath(LOCAL_CONFIG_PATH), JSON.stringify(actorJson, null, '\t'), { flag: 'w' }); | ||
| }; | ||
|
|
||
| describe('apify actor calculate-memory', () => { | ||
|
Contributor
Author
There was a problem hiding this comment. @vladfrangu when I start it one by one it works perfectly, but when I start it as as a whole set it fails, can you help me to fix that, please? 🙃 It seems there's something wrong with path or existence of the file. I was a bit confused with that |
||
| const START_URLS_LENGTH_BASED_MEMORY_EXPRESSION = "get(input, 'startUrls.length', 1) * 1024"; | ||
| const DEFAULT_INPUT = { startUrls: [1, 2, 3, 4] }; | ||
|
|
||
| const inputPath = joinPath(getLocalKeyValueStorePath('default'), 'INPUT.json'); | ||
|
|
||
| beforeAll(async () => { | ||
| mkdirSync(dirname(inputPath), { recursive: true }); | ||
| writeFileSync(inputPath, JSON.stringify(DEFAULT_INPUT), { flag: 'w' }); | ||
| await beforeAllCalls(); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| await afterAllCalls(); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| resetCwdCaches(); | ||
| }); | ||
|
|
||
| it('should fail when default memory is not provided in flags or actor.json', async () => { | ||
| await createActorJson(); | ||
|
|
||
| await testRunCommand(ActorCalculateMemoryCommand, {}); | ||
|
|
||
| expect(lastErrorMessage()).toMatch(/No memory-calculation expression found./); | ||
| }); | ||
|
|
||
| it('should calculate memory using defaultMemoryMbytes flag', async () => { | ||
| await testRunCommand(ActorCalculateMemoryCommand, { | ||
| flags_input: inputPath, | ||
| flags_defaultMemoryMbytes: START_URLS_LENGTH_BASED_MEMORY_EXPRESSION, | ||
| }); | ||
|
|
||
| expect(lastLogMessage()).toMatch(/4096 MB/); | ||
| }); | ||
|
|
||
| it('should calculate memory using expression from actor.json', async () => { | ||
| await createActorJson({ defaultMemoryMbytes: START_URLS_LENGTH_BASED_MEMORY_EXPRESSION }); | ||
|
|
||
| await testRunCommand(ActorCalculateMemoryCommand, { | ||
| flags_input: inputPath, | ||
| }); | ||
|
|
||
| expect(lastLogMessage()).toMatch(/4096 MB/); | ||
| }); | ||
|
|
||
| it('should fallback to default input path if input flag is not provided', async () => { | ||
| await createActorJson({ defaultMemoryMbytes: START_URLS_LENGTH_BASED_MEMORY_EXPRESSION }); | ||
|
|
||
| await testRunCommand(ActorCalculateMemoryCommand, {}); | ||
|
|
||
| expect(lastLogMessage()).toMatch(/4096 MB/); | ||
| }); | ||
|
|
||
| it('should report error if memory calculation expression is invalid', async () => { | ||
| await createActorJson({ defaultMemoryMbytes: 'invalid expression' }); | ||
|
|
||
| await testRunCommand(ActorCalculateMemoryCommand, { | ||
| flags_defaultMemoryMbytes: 'invalid expression', | ||
| }); | ||
|
|
||
| expect(lastErrorMessage()).toMatch(/Memory calculation failed: /); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why these changes? They seem unrelated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was caused by automatic doc generation, so I think it is fine