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

Replace Base.cp with run(...). #69

Merged
merged 1 commit into from
Nov 13, 2024
Merged

Conversation

sriharshakandala
Copy link
Member

@sriharshakandala sriharshakandala commented Nov 12, 2024

Replace Base.cp with run(...) on non-Windows operating systems.
Base.cp seems to have a bug when copying large files. Using run fixes this behaviour.
This fixes the bug mentioned in #64

Checklist:

  • I created a new folder $artifact_name
    • I added a README.md in that that folder that
      • describes the data and processing done to it
      • lists the sources of the raw data
      • lists the required citation, licenses
    • If applicable (e.g., for Creative Commons), I added a LICENSE file
    • I added the scripts that retrieve, process, and produce the artifact
    • I added the environment used for such scripts (typically, Project.toml
      and Manifest.toml)
    • I added the OutputArtifacts.toml file containing the information
      needed for package developers to add $artifact_name to their package
  • I uploaded the artifact folder to the Caltech cluster (in
    /groups/esm/ClimaArtifacts/artifacts/$artifact_name)
  • I added the relevant code to the Overides.toml on the Caltech Cluster
    (in /groups/esm/ClimaArtifacts/artifacts/Overrides.toml)
  • I added a link to the main README.md to point to the new artifact

@Sbozzolo
Copy link
Member

I think that this would break compatibility with windows

@sriharshakandala
Copy link
Member Author

One option is to merge this for now, revert to Base.cp once the bug is fixed in Julia Base! Most of the systems we use are either Linux or MacOS.

@Sbozzolo
Copy link
Member

I think we can check for windows and use Base.cp in that case.

@charleskawczynski
Copy link
Member

I think we can check for windows and use Base.cp in that case.

That's what I was thinking, too. I'm trying to run the example with Base.cp(file_path, joinpath(output_dir, basename(file_path))) on my windows machine and I have the Abort this calculation, unless you know what you are doing. warning, and it kind of seems like it's hanging (or taking a long time to download). Maybe we can add an additional warning if this is run on windows?

@Sbozzolo
Copy link
Member

I think we can check for windows and use Base.cp in that case.

That's what I was thinking, too. I'm trying to run the example with Base.cp(file_path, joinpath(output_dir, basename(file_path))) on my windows machine and I have the Abort this calculation, unless you know what you are doing. warning, and it kind of seems like it's hanging (or taking a long time to download). Maybe we can add an additional warning if this is run on windows?

It takes a long time downloading. The way I was monitoring it is by looking at the temporary file that gets created by Downloads (haing a better progress report would be very nice...).

The warning is because you probably already have something in your directory. For this artifact (made of only one download file), it is harmless.

@sriharshakandala sriharshakandala force-pushed the sk/replace_basedotcp branch 2 times, most recently from 793ec0a to 05beb1c Compare November 13, 2024 22:00
`Base.cp` seems to have a bug when copying large files. Using `run` fixes this behaviour.
This fixes the bug mentioned in #64
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants