Skip to content

Fix: Workflow maintaing conda environment#60

Merged
oelbert merged 2 commits into
NOAA-GFDL:developfrom
fmalatino:workflow_fix
Sep 15, 2025
Merged

Fix: Workflow maintaing conda environment#60
oelbert merged 2 commits into
NOAA-GFDL:developfrom
fmalatino:workflow_fix

Conversation

@fmalatino

@fmalatino fmalatino commented Sep 11, 2025

Copy link
Copy Markdown
Contributor

Description
This PR ensures that the same shell is opened across workflow steps and no longer calls for removal of sqlite after installation.

🚧
To be merged after PR 146 in Pace

Fixes:
Current issues in the workflow

How Has This Been Tested?
Tested with the current set of unit tests

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included
  • Targeted model, if this change was triggered by a model need/shortcoming

@fmalatino fmalatino marked this pull request as ready for review September 11, 2025 20:21
@bensonr bensonr self-requested a review September 11, 2025 20:25
bensonr
bensonr previously approved these changes Sep 11, 2025

@bensonr bensonr left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we need to be careful about specifying bash as its behavior can change as we update the underlying container or system OS.

jjuyeonkim
jjuyeonkim previously approved these changes Sep 11, 2025

@jjuyeonkim jjuyeonkim left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me -- similar to NOAA-GFDL/pace#146.

Comment thread .github/workflows/translate.yaml Outdated

- name: Orchestrated dace:cpu Translate Test
shell: bash -l {0}
shell: bash -el {0}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this shell specification still necessary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, without it the python installed in the conda environment cannot be accessed.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All other steps seem to be fine with the default run shell specification in lines 29-31. How is this step different?
I guess I'm missing something obvious here ... just asking cause I'm curious how this works.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh...I totally forgot to remove this, thanks!

@fmalatino

Copy link
Copy Markdown
Contributor Author

I think we need to be careful about specifying bash as its behavior can change as we update the underlying container or system OS.

We can add other shells to test when needed, as is depicted in this documentation.

@oelbert oelbert added this pull request to the merge queue Sep 15, 2025
Merged via the queue into NOAA-GFDL:develop with commit 446cf44 Sep 15, 2025
3 checks passed
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.

5 participants