-
Notifications
You must be signed in to change notification settings - Fork 552
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
Scheduler/default scheduled #4871
base: develop
Are you sure you want to change the base?
Changes from 7 commits
571bde5
36e03ed
ef91d88
da4e7d1
6d00568
f30edfd
d9876be
3db4e44
d4ae3f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
|
||
import logging | ||
from datetime import datetime | ||
from fiftyone.internal.util import is_remote_service | ||
|
||
from fiftyone.operators.executor import ( | ||
ExecutionContext, | ||
|
@@ -36,17 +37,21 @@ def __init__( | |
) | ||
self.run_state = ( | ||
ExecutionRunState.QUEUED | ||
) # default to queued state on create | ||
if is_remote_service() | ||
else ExecutionRunState.SCHEDULED | ||
) # if running locally use SCHEDULED otherwise QUEUED | ||
self.run_link = None | ||
self.queued_at = datetime.utcnow() | ||
self.queued_at = datetime.utcnow() if is_remote_service() else None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider caching the result of The function Apply this diff to modify the code: + is_remote = is_remote_service()
self.run_state = (
ExecutionRunState.QUEUED
- if is_remote_service()
+ if is_remote
else ExecutionRunState.SCHEDULED
) # if running locally use SCHEDULED otherwise QUEUED
self.run_link = None
- self.queued_at = datetime.utcnow() if is_remote_service() else None
+ self.queued_at = datetime.utcnow() if is_remote else None self.scheduled_at = (
- datetime.utcnow() if not is_remote_service() else None
+ datetime.utcnow() if not is_remote else None
) Add the following line at the beginning of the is_remote = is_remote_service() Also applies to: 52-54 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we will want to inject these values in, instead of determining them inside the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @swheaton, understood. Injecting these values into the 🐰🤔 ✏️ Learnings added
|
||
self.updated_at = datetime.utcnow() | ||
self.status = None | ||
self.dataset_id = None | ||
self.started_at = None | ||
self.pinned = False | ||
self.completed_at = None | ||
self.failed_at = None | ||
self.scheduled_at = None | ||
self.scheduled_at = ( | ||
datetime.utcnow() if not is_remote_service() else None | ||
) | ||
self.result = None | ||
self.id = None | ||
self._doc = None | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,3 +7,4 @@ | |
""" | ||
|
||
from .secrets import * | ||
from .util import is_remote_service |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
""" | ||
FiftyOne internal utilities. | ||
|
||
| Copyright 2017-2024, Voxel51, Inc. | ||
| `voxel51.com <https://voxel51.com/>`_ | ||
| | ||
""" | ||
|
||
|
||
def is_remote_service(): | ||
"""Whether the SDK is running in a remote service context. | ||
|
||
Returns: | ||
True/False | ||
""" | ||
return has_encryption_key() and has_api_key() | ||
|
||
|
||
def has_encryption_key(): | ||
"""Whether the current environment has an encryption key. | ||
|
||
Returns: | ||
True/False | ||
""" | ||
return False | ||
|
||
|
||
def has_api_key(): | ||
"""Whether the current environment has an API key. | ||
|
||
Returns: | ||
True/False | ||
""" | ||
return False |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file should not change. We will be executing queued operations as always. |
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 is backwards. queued for local run, scheduled for remote service.
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.
oh yup my bad for some reason got it swapped again thinking about how we had it before