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

clean up ci and gpu scaling config files #2948

Merged
merged 1 commit into from
Apr 29, 2024
Merged

clean up ci and gpu scaling config files #2948

merged 1 commit into from
Apr 29, 2024

Conversation

szy21
Copy link
Member

@szy21 szy21 commented Apr 24, 2024

Purpose

Currently we have config files under gpu_configs that are used for both ci and GPU scaling pipeline. I find it a bit confusing when trying to find the files and add new files. This PR moves GPU scaling pipeline config files to a new folder gpu_scaling_configs.

This is probably a breaking change as I think the coupler assumes some path of atmos configs. cc @juliasloan25

To-do

Content


  • I have read and checked the items on the review checklist.

@szy21
Copy link
Member Author

szy21 commented Apr 24, 2024

Hmm, some configs are used in both ci and GPU scaling pipeline. That means we should probably clean it up - if it's already running in ci then we don't need to run it in weekly scaling pipeline again.

Copy link
Member

@charleskawczynski charleskawczynski left a comment

Choose a reason for hiding this comment

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

I thought that there was a difference between the gpu and scaling pipelines. I'm going to add @sriharshakandala as a reviewer for this

@sriharshakandala
Copy link
Member

sriharshakandala commented Apr 24, 2024

Hmm, some configs are used in both ci and GPU scaling pipeline. That means we should probably clean it up - if it's already running in ci then we don't need to run it in weekly scaling pipeline again.

The GPU scaling pipeline runs on the A100s. This may not be the case with the GPU runs on the standard CI pipeline, which most likely uses a P100 GPU!

@szy21
Copy link
Member Author

szy21 commented Apr 24, 2024

Yes, I guess it’s ok to run it twice for now. I mostly want to reorganize the files in this PR. We can clean up the jobs later.

@szy21
Copy link
Member Author

szy21 commented Apr 24, 2024

@sriharshakandala are you ok with this folder change, or do you want to keep all of them in the same folder?

@juliasloan25
Copy link
Member

Could you please add an entry to the NEWS.md for this PR?

@szy21 szy21 removed the API label Apr 24, 2024
@sriharshakandala
Copy link
Member

@sriharshakandala are you ok with this folder change, or do you want to keep all of them in the same folder?

One way is to keep the gpu_pipeline configs in gpu_configs (or rename that to gpu_pipeline_configs) and the CI GPU configs in model_configs! For the CI runs, we can use a smaller integration time, just to check if everything is running correctly. We may want to use a reasonably large integration time for the gpu_pipeline for accurate performance measurements.

@szy21
Copy link
Member Author

szy21 commented Apr 24, 2024

@sriharshakandala are you ok with this folder change, or do you want to keep all of them in the same folder?

One way is to keep the gpu_pipeline configs in gpu_configs (or rename that to gpu_pipeline_configs) and the CI GPU configs in model_configs! For the CI runs, we can use a smaller integration time, just to check if everything is running correctly. We may want to use a reasonably large integration time for the gpu_pipeline for accurate performance measurements.

Ok, I can do that!

@szy21
Copy link
Member Author

szy21 commented Apr 24, 2024

@sriharshakandala could you look at it again? @juliasloan25 I don't think it's a breaking change anymore if I change it this way (unless you use anything in model_configs in the coupler), do you still want it to be in the NEWS?

@juliasloan25
Copy link
Member

@juliasloan25 I don't think it's a breaking change anymore if I change it this way (unless you use anything in model_configs in the coupler), do you still want it to be in the NEWS?

We don't use Atmos's model_configs, so it's not a breaking change but it would still be nice to include in the NEWS :)

@szy21 szy21 force-pushed the zs/cleanup_ci branch 3 times, most recently from 1d0b27a to da2eaae Compare April 29, 2024 05:32
@szy21 szy21 enabled auto-merge April 29, 2024 16:07
@szy21 szy21 added this pull request to the merge queue Apr 29, 2024
Merged via the queue into main with commit f4da67f Apr 29, 2024
11 checks passed
@szy21 szy21 deleted the zs/cleanup_ci branch April 29, 2024 21:42
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