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

Add LocalStorage Implementation #13981

Merged
merged 15 commits into from
Jun 20, 2024
Merged

Add LocalStorage Implementation #13981

merged 15 commits into from
Jun 20, 2024

Conversation

masonmenges
Copy link
Contributor

@masonmenges masonmenges commented Jun 12, 2024

This pr introduces a LocalStorage implementation to be used with RunnerDeployments for flow deployed in a local process, this closes a gap sometimes felt by users when using the deploy method to deploy prefect flows instead of serve or through a prefect yaml.

closes #13499

Example

from prefect import flow, get_run_logger

@flow
def test_flow():
    logger = get_run_logger()
    logger.info("This is a test")


if __name__ == "__main__":
    test_flow.from_source(
        source="/Users/masonmenges/Repos/git_hub_repos/sandbox/mm2-sanbox/flows/",
        entrypoint="local_dev_flow.py:test_flow"
    ).deploy(
        name="local_deployment", 
        work_pool_name="default"
        )

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • This pull request includes a label categorizing the change e.g. maintenance, fix, feature, enhancement, docs.

For documentation changes:

  • This pull request includes redirect settings in mint.json for files that are removed or renamed.

For new functions or classes in the Python SDK:

  • This pull request includes helpful docstrings.
  • If a new Python file was added, this pull request contains a stub page in the Python SDK docs and an entry in docs/mint.json navigation.

@masonmenges masonmenges added the enhancement An improvement of an existing feature label Jun 12, 2024
@masonmenges masonmenges changed the title Local storage Add LocalStorage Implementation Jun 13, 2024
@masonmenges masonmenges marked this pull request as ready for review June 13, 2024 19:25
@masonmenges masonmenges requested a review from a team as a code owner June 13, 2024 19:25
Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @masonmenges! In addition to the comments I left, can you add some more tests for the new LocalStorage class and tests covering the changes to create_storage_from_url?

src/prefect/flows.py Outdated Show resolved Hide resolved
Comment on lines 608 to 620
async def pull_code(self):
"""
Pulls the contents of the configured repository to the local filesystem.
"""
self._logger.debug(
"Checking local file path '%s'...",
self.destination,
)

local_dir = self.destination

if local_dir.exists():
os.chdir(local_dir)
Copy link
Member

Choose a reason for hiding this comment

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

For LocalStorage, we want pull_code to be a noop. Changing the directory here will change the current working directory for the entire process, which would break other flows served by the Runner if they're in other directories.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this should just pass now and not doing anything else.

src/prefect/runner/storage.py Outdated Show resolved Hide resolved
@@ -582,8 +655,12 @@ def create_storage_from_url(
parsed_url = urlparse(url)
if parsed_url.scheme == "git" or parsed_url.path.endswith(".git"):
return GitRepository(url=url, pull_interval=pull_interval)
else:
elif parsed_url.scheme in fsspec.available_protocols():
Copy link
Member

Choose a reason for hiding this comment

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

I think we might want to check for file and local schemes before this check to ensure we don't create a RemoteStorage instance if a user passes in file://path/to/flow.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm splitting the path that gets passed in now on these characters "://" when the scheme is local or file but I'm not sure if there's maybe a better way to handle that though since I think that can get kinda tricky when you start including drive letters into the mix for windows paths.

@masonmenges
Copy link
Contributor Author

@desertaxle Ok I think I've got most of this in a good spot now and added some more tests for local storage, I'm still a little unsure of how I'm handling the local and file schemes though so if you have any thoughts there I would greatly appreciate it.

src/prefect/runner/storage.py Outdated Show resolved Hide resolved
src/prefect/runner/storage.py Outdated Show resolved Hide resolved
src/prefect/runner/storage.py Outdated Show resolved Hide resolved
"""
step = {
"prefect.deployments.steps.set_working_directory": {
"directory": self.destination
Copy link
Member

Choose a reason for hiding this comment

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

The destination needs to be cast to a string to ensure it can be saved correctly in the DB.

Suggested change
"directory": self.destination
"directory": str(self.destination)

Copy link
Contributor

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

couple comments!

src/prefect/runner/storage.py Outdated Show resolved Hide resolved
src/prefect/runner/storage.py Show resolved Hide resolved
Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

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

LGTM! Please be sure to port these changes to main also!

@masonmenges masonmenges merged commit df37fa0 into 2.x Jun 20, 2024
55 checks passed
@masonmenges masonmenges deleted the local_storage branch June 20, 2024 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants