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

Migrate setup configurations to pyproject.toml #1043

Closed
wants to merge 2 commits into from

Conversation

altheaden
Copy link
Collaborator

@altheaden altheaden commented Dec 3, 2024

This PR moves setup procedures from the setup.py and setup.cfg workflow to a single pyproject.toml file.

Checklist

  • Documentation has been built locally and changes look as expected
  • Testing comment in the PR documents testing used to verify the changes

@altheaden
Copy link
Collaborator Author

Testing

I was able to create an environment and also build the documentation with this setup without any issues. I'm not sure if there's any additional testing that can be done here, but I'm happy to do it if there is.

@altheaden altheaden requested a review from xylar December 3, 2024 16:42
@xylar
Copy link
Collaborator

xylar commented Dec 3, 2024

I will run the test suite on Chrysalis to make sure things are fine.

Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

A few small aesthetic fixes.

force_grid_wrap = "0"
use_parentheses = true
line_length = "79"
skip = ["e3sm_diags/e3sm_diags_driver.py"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be removed. (Also wherever it got copied from if that's not e3sm_diags, which I suspect is the case.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you just mean the last line that references e3sm_diags or should the three lines about formatting also be removed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I only selected the one line so I don't know why it shows 4. Yes, I just mean the last one. Thanks for asking.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
skip = ["e3sm_diags/e3sm_diags_driver.py"]

Comment on lines +38 to +44


"Development Status :: 5 - Production/Stable",
"License :: OSI Approved :: BSD License",
"Operating System :: OS Independent",
"Intended Audience :: Science/Research",
"Topic :: Scientific/Engineering",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some whitespace issues:

Suggested change
"Development Status :: 5 - Production/Stable",
"License :: OSI Approved :: BSD License",
"Operating System :: OS Independent",
"Intended Audience :: Science/Research",
"Topic :: Scientific/Engineering",
"Development Status :: 5 - Production/Stable",
"License :: OSI Approved :: BSD License",
"Operating System :: OS Independent",
"Intended Audience :: Science/Research",
"Topic :: Scientific/Engineering",

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops, looks like I accidentally mixed tabs and spaced. I'll fix that.

Comment on lines +25 to +27
description = """\
Analysis for Model for Prediction Across Scales (MPAS) simulations.\
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description = """\
Analysis for Model for Prediction Across Scales (MPAS) simulations.\
"""
description = """\
Analysis for Model for Prediction Across Scales (MPAS) simulations.\
"""

@xylar
Copy link
Collaborator

xylar commented Dec 3, 2024

Testing

I ran the test suite on this branch. I had no trouble creating the mpas_dev environment from the new pyproject.toml and the results for the tests that have completed look exactly as expedted:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.xasay-davis/analysis_testing/chrysalis/migrate-to-pyproj/

I will double check that all tests complete before merging.

@xylar
Copy link
Collaborator

xylar commented Dec 3, 2024

GitHub seems to be glitching. I got an email that you pushed a commit and I can see it directly but it isn't showing up here.

@xylar
Copy link
Collaborator

xylar commented Dec 3, 2024

Assuming it's working tomorrow, I'll approve and merge. The changes look great!

@altheaden
Copy link
Collaborator Author

Yep, I'm having the same issues. Sounds good!

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.

2 participants