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

Check if from_path and local_path are the same in LocalFileSystem.get… #6824

Merged
merged 4 commits into from
Sep 15, 2022
Merged

Check if from_path and local_path are the same in LocalFileSystem.get… #6824

merged 4 commits into from
Sep 15, 2022

Conversation

jmg-duarte
Copy link
Contributor

@jmg-duarte jmg-duarte commented Sep 14, 2022

Fixes #6652

Check if the resolved from_path and local_path are the same, if yes, do not copy anything.

Example

get_directory(".", ".") # no longer throws an error

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.
  • This pull request includes tests or only affects documentation.
  • This pull request includes a fix, feature, enhancement, docs, collections, or maintenance label categorizing the change.

@netlify
Copy link

netlify bot commented Sep 14, 2022

Deploy Preview for prefect-orion ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 954c8e3
🔍 Latest deploy log https://app.netlify.com/sites/prefect-orion/deploys/6322e0acb1198c0008ff404d
😎 Deploy Preview https://deploy-preview-6824--prefect-orion.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@zanieb zanieb added the fix A fix for a bug in an existing feature label Sep 14, 2022
@zanieb
Copy link
Contributor

zanieb commented Sep 14, 2022

This makes sense to me! Thanks for contributing :)

Can you write a test that covers this change please?

@jmg-duarte
Copy link
Contributor Author

@madkinsz done!

@jmg-duarte
Copy link
Contributor Author

jmg-duarte commented Sep 14, 2022

@madkinsz while I have your attention, I'd like to question why is Prefect copying the files anyway. It just doesn't seem necessary given that the code is already inside the container (in the context of #6652)

@zanieb
Copy link
Contributor

zanieb commented Sep 14, 2022

@madkinsz while I have your attention, I'd like to question why is Prefect copying the files anyway. It just doesn't seem necessary given that the code is already inside the container (in the context of #6652)

I presume this is just following a consistent code path for simplicity of implementation, but I'm not the current expert on this section. With this change it'll just be a no-op, correct?

@jmg-duarte
Copy link
Contributor Author

I presume this is just following a consistent code path for simplicity of implementation, but I'm not the current expert on this section. With this change it'll just be a no-op, correct?
Yes, this change makes it a no-op. I'm just curious why in general it works as it does.

@zanieb
Copy link
Contributor

zanieb commented Sep 14, 2022

FYI there are some brief notes on using pre-commit or installing our required black version in https://docs.prefect.io/contributing/overview/#setting-up-a-development-environment

@jmg-duarte
Copy link
Contributor Author

@madkinsz I know, I was just lazy because I was using github.dev the issue was also very hard to notice just by the logs, just needed some whitespace trimming (which I have by default on my local editor 🤦). Anyway, I did things proper now, so it should be ok

@zanieb
Copy link
Contributor

zanieb commented Sep 15, 2022

Thanks again!

@zanieb zanieb merged commit d24db49 into PrefectHQ:main Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix A fix for a bug in an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Docker Container with imbedded flows throws error when loading while parsing directory
2 participants