-
Notifications
You must be signed in to change notification settings - Fork 38
feat(actors): enables standby mode via actor.json #991
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
base: master
Are you sure you want to change the base?
Conversation
|
Keeping this as draft, it is working but it seems we missing some types in apify-client, lets discuss. |
| // Default standby mode configuration when usesStandbyMode is enabled in actor.json | ||
| const DEFAULT_STANDBY_OPTIONS = { | ||
| isEnabled: true, | ||
| disableStandbyFieldsOverride: false, | ||
| maxRequestsPerActorRun: 4, | ||
| desiredRequestsPerActorRun: 3, | ||
| idleTimeoutSecs: 300, | ||
| build: 'latest', | ||
| memoryMbytes: 1024, | ||
| shouldPassActorInput: false, | ||
| }; | ||
|
|
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.
I stole this from console.apify.com, it is default preset send when user just enable standby mode. cc @vladfrangu
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.
my idea was result should be the same no matter if I use CLI or console UI
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.
I don't like that we had to copy this from ui, because if they change we'll miss them on our end... Maybe we can see if this can be moved to apify/consts?
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.
@vladfrangu or we can change a actor.json parameter from usesStandbyMode to actorStandBy (matching apify client interface) but user has to provide all required parameters (only disableStandbyFieldsOverride is optional) 🤔
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.
I would just skip this honestly. The defaults should be managed by the API. Copying them to the CLI is wrong, but so is exporting them via shared-js.
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.
So how about creating the Actor without the standby mode flag and then issuing another call to make it standby? By then, you will be able to fetch its current (defaultified) attributes and use them if the API will need you to parrot them back...
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.
I tried it right now, without standby mode flag, I will not receive actorStandby object at all :/
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.
Let's bug the core services people about it then?
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.
Let's bug the core services people about it then?
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.
I did, they have defaults hardcoded in FE, I guess I will change the schema for actor.json so user can provide the whole object with all required parameters. WDYT @vladfrangu ?
src/commands/actors/push.ts
Outdated
|
|
||
| // Enable standby mode if configured in actor.json | ||
| if (actorConfig!.usesStandbyMode) { | ||
| newActor.actorStandby = DEFAULT_STANDBY_OPTIONS as ActorCollectionCreateOptions['actorStandby']; |
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.
ActorCollectionCreateOptions type look little bit out of date -> StandBy type missing disableStandbyFieldsOverride and shouldPassActorInput
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.
Please add this to the issue with outdated types in client-js 🙏
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.
|
lest's wait for #992 merged and then rebase |
Enables standby mode for Actors based on the 'usesStandbyMode' property in actor.json. If the property is set to true during Actor creation or update, standby mode is enabled with default configuration. Fixes #913
1413949 to
dd47acb
Compare
janbuchar
left a comment
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.
It's great that you are fixing this! Consider this approved by me once @vladfrangu approves it, I don't have anything to add beside that one comment.
| // Default standby mode configuration when usesStandbyMode is enabled in actor.json | ||
| const DEFAULT_STANDBY_OPTIONS = { | ||
| isEnabled: true, | ||
| disableStandbyFieldsOverride: false, | ||
| maxRequestsPerActorRun: 4, | ||
| desiredRequestsPerActorRun: 3, | ||
| idleTimeoutSecs: 300, | ||
| build: 'latest', | ||
| memoryMbytes: 1024, | ||
| shouldPassActorInput: false, | ||
| }; | ||
|
|
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.
I would just skip this honestly. The defaults should be managed by the API. Copying them to the CLI is wrong, but so is exporting them via shared-js.
Fix
usesStandbyModenot being applied when pushing Actor via CLIProblem
When creating an Actor locally using
apify create my-actor -t ts-mcp-serverand then pushing it withapify push, theusesStandbyMode: truesetting fromactor.jsonwas being ignored. The Actor would be created on the platform without standby mode enabled, even though the template explicitly sets this option.In contrast, cloning the same template directly from the Apify Console UI correctly enabled standby mode.
Root Cause
The
pushcommand was not reading theusesStandbyModeproperty fromactor.jsonand was not passing it to the Apify API when creating or updating Actors.Solution
DEFAULT_STANDBY_OPTIONSconstant with all required API fields for standby mode configurationusesStandbyMode: trueis set inactor.json, the standby options are passed to the APIusesStandbyMode: trueis inactor.json, the Actor is updated to enable standby modeChanges
src/commands/actors/push.ts: Added standby mode support for both new and existing Actorstest/api/commands/push.test.ts: Added two API tests to verify standby mode is correctly enabledDefault Standby Configuration
When
usesStandbyMode: trueis set, the following defaults are applied:isEnabledtruedisableStandbyFieldsOverridefalsemaxRequestsPerActorRun4desiredRequestsPerActorRun3idleTimeoutSecs300build'latest'memoryMbytes1024shouldPassActorInputfalseTesting
apify create my-actor -t ts-mcp-server cd my-actor apify pushThe Actor should now have standby mode enabled with the default configuration.
closes #913