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

Issue #2016 update the Python versions used by the METplus analysis t… #2044

Merged
merged 3 commits into from
Feb 16, 2023

Conversation

bikegeek
Copy link
Contributor

@bikegeek bikegeek commented Feb 9, 2023

…ools

Pull Request Testing

  • Describe testing already performed for these changes:

    verify that in the ReadTheDocs User's Guide, the following versions are specified for Python 3.8.x:

lxml>=4.9.1
matplotlib >=3.5.2
metpy>=1.3.1
netcdf4?=1.6.0
numpy>=1.23.5
pandas>=1.5.1
pint>=0.19.2
plotly>=5.9.0
pymysql>=1.0.2
pytest>=7.2.0
pyyaml>=6.0
scikit-image>=0.19.3
scipy>=1.8.1

  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

Do the same

  • Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [NA]

  • Do these changes include sufficient testing updates? [NA]

  • Will this PR result in changes to the test suite? [No]

    If yes, describe the new output and/or changes to the existing output:

  • Please complete this pull request review by Bugfix release.

Pull Request Checklist

See the METplus Workflow for details.

  • Add any new Python packages to the METplus Components Python Requirements table.
  • Review the source issue metadata (required labels, projects, and milestone).
  • Complete the PR definition above.
  • Ensure the PR title matches the feature or bugfix branch name.
  • Define the PR metadata, as permissions allow.
    Select: Reviewer(s)
    Select: Organization level software support Project or Repository level development cycle Project
    Select: Milestone as the version that will include these changes
  • After submitting the PR, select Development issue with the original issue number.
  • After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge.
  • Close the linked issue and delete your feature or bugfix branch from GitHub.

@bikegeek bikegeek added this to the METplus-5.0.1 Bugfix milestone Feb 9, 2023
@bikegeek bikegeek linked an issue Feb 9, 2023 that may be closed by this pull request
22 tasks
Copy link
Collaborator

@jprestop jprestop left a comment

Choose a reason for hiding this comment

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

Hi @bikegeek. Thank you for all of your work on this task. For some of the versions changed below, I see some discrepancies in the various requirements files. I'll put comments directly on those items below. There are some other packages that weren't changed that I have some questions about, which I'll list below in the comments here:

cartopy>=0.18.0 - I don't see it in the requirements.txt file for METcalcpy or METplotpy. If it is not needed, please remove METcalcpy or METplotpy from the "METplus component" column. It looks like this is still used in METplus wrappers for use cases.

cmocean - I don't see it in the requirements.txt file for METcalcpy or METplotpy. If it is not being used, please remove this row entirely.

eofs - I don't see it in the requirements.txt file for METcalcpy or METplotpy. If it is not needed, please remove METcalcpy or METplotpy from the "METplus component" column. It looks like this is still used in METplus wrappers for use cases.

nc-time-axis 1.4 - I don't see it in the requirements.txt file for METcalcpy or METplotpy. If it is not being used, please remove this row entirely.

scikit-learn 0.23.2 - I don't see it in the requirements.txt file for METcalcpy or METplotpy. If it is not needed, please remove METcalcpy or METplotpy from the "METplus component" column. It looks like this is still used in METplus wrappers for use cases.

yaml - Is this the same as pyyaml? I see that pyyaml lists https://github.com/yaml/pyyaml as the source by yaml lists https://pypi.org/project/PyYAML/. If they are the same, please pick whichever line is most accurate and delete the other.

@@ -223,7 +223,7 @@ METplus Components Python Requirements
and much more easier
-
* - imageio
-
- >=2.19.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is listed as 2.19.3 in plotpy requirements.txt, but 2.6.1 in calcpy requirements.txt. Should we update the calcpy requirements.txt file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created issue in METcalcpy to update the versions in the requirements.txt:
dtcenter/METcalcpy#276

@@ -349,7 +349,7 @@ METplus Components Python Requirements
(metplotpy)
<../generated/model_applications/s2s/TCGen_fcstGFSO_obsBDECKS_GDF_TDF.html>`_
* - metpy
-
- >=1.3.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is listed as 1.3.1 in calcpy and plotpy nco_requirements.txt, but as 1.1.0 in calcpy requirements.txt. Should we update the calcpy requirements.txt file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -371,7 +371,7 @@ METplus Components Python Requirements
\**REQUIRES Python 3.7
-
* - netCDF4
- >=1.5.4
- >=1.6.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is listed as 1.6.0 in METplotpy nco_requirements.txt and in plotpy requirements.txt, but 1.5.7 in calcpy requirements.txt and 1.6.2 in calcpy nco_requirements.txt. Should we update the calcpy requirements.txt and calcpy nco_requirements.txt or should the value here be 1.6.2 and should we update everywhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's prudent to update to the latest version, so let's keep 1.6.2 and update in METplotpy and METcalcpy. I will put a note in METplotpy dtcenter/METplotpy#310 (update to Python 3.10 to use netCDF 1.6.2)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bikegeek I defer to you on this one. If that is what you think is best, that works for me. This is PR approved. :)

@@ -380,7 +380,7 @@ METplus Components Python Requirements
the netCDF C library
- For using MET Python embedding functionality in use cases
* - numpy
- >=1.19.2
- >=1.23.5
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is listed as 1.22.0 in nco_requirements.txt, 1.22.3 in calcpy requirements.txt. I'm not sure which one we need and if we should update elsewhere.

@@ -394,7 +394,7 @@ METplus Components Python Requirements
Fourier transforms, and more.
- For using MET Python embedding functionality in use cases
* - pandas
- >=1.0.5, <=1.2.3 (METdataio)
- >=1.5.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

1.5.1 in calcpy, plotpy and dataio nco_requirements.txt and 1.5.1 in plotpy and dataio requirements.txt, but 1.2.3 in calcpy requirements.txt. Should we update in calcpy requirements.txt?

@@ -409,15 +409,15 @@ METplus Components Python Requirements
language
- For using MET Python embedding functionality in use cases
* - pint
- >=0.18
- >=0.19.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is listed as 0.19.2 in calcpy and plotpy nco_requirements.txt and in plotpy requirements.txt, but 0.18 in calcpy requirements.txt. Should we update calcpy requirements.txt?

@@ -528,7 +528,7 @@ METplus Components Python Requirements
libraries like Plotly
-
* - pyyaml
- >=5.3.1
- >=6.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is listed as 6.0 in calcpy, plotpy, and dataio nco_requirements.txt and 6.0 in plotpy and dataio requirements.txt, but 5.4.1 in calcpy requirements.txt. Should we update calcpy requirements.txt?

@@ -538,7 +538,7 @@ METplus Components Python Requirements
programming language
-
* - scikit-image
- >=0.16.2
- >=0.19.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is listed as 0.18.1 in calcpy requirements.txt. Should we updated calcpy requirements.txt?

@@ -572,7 +572,7 @@ METplus Components Python Requirements
(scikit-learn)
<../generated/model_applications/marine_and_cryosphere/GridStat_fcstRTOFS_obsSMAP_climWOA_sss.html>`_
* - scipy
- >=1.5.1
- >=1.8.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is listed as 1.8.1 in calcpy and plotpy nco_requirements.txt and 1.8.1 in plotpy requirements, but 1.8.0 in calcpy requirements.txt. Should we update calcpy requirements

@bikegeek
Copy link
Contributor Author

bikegeek commented Feb 10, 2023 via email

@bikegeek
Copy link
Contributor Author

bikegeek commented Feb 10, 2023 via email

@jprestop
Copy link
Collaborator

The METcalcpy requirements did not get updated for the bugfix release (as
decided during one of the weekly meetings), so that will need to be
updated. But we will also need to update again for the Python 3.10, which
is why I think it was decided not to update it for the bugfix release???

Ah! I see. I totally missed that or forgot about it. Thank you for letting me know.

@jprestop
Copy link
Collaborator

cmocean, scikit-learn, and eofs are in the S2S use cases so will need to be
installed if running those use cases. Cartopy is used by the physics tendency use case (under the short-range
examples). Are these requirements just for NCO, or for all users of
METplus?

Great question. No, they are not just for NCO, however, I saw METcalcpy and METplotpy listed in the "METplus Component" section so I was thinking that METcalcpy and METplotpy needed to be removed if that requirement was not for those components but was for METplus wrappers, but perhaps they're all intertwined? I really have no idea and defer to you on this. It just caught my attention so I thought I should mention it.

Do you think this is all ok then? The only one I really wasn't sure about was netCDF4>=1.6.0. I thought maybe with the updates for NCO it needed to be 1.6.2 since it was 1.6.2 in calcpy nco_requirements.txt.

@bikegeek
Copy link
Contributor Author

bikegeek commented Feb 10, 2023 via email

Copy link
Collaborator

@jprestop jprestop left a comment

Choose a reason for hiding this comment

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

Great, thank you! Once netCDF4 is updated to 1.6.2 I approve this request.

@bikegeek bikegeek marked this pull request as draft February 14, 2023 21:12
@bikegeek bikegeek marked this pull request as ready for review February 14, 2023 21:12
@jprestop
Copy link
Collaborator

Hi @bikegeek. I just wanted to ensure that you knew that this is approve and ready for squashing and merging. According to the Merging pull requests section of our Contributor's Guide "As permissions allow, the requestor is responsible for merging the pull request once it has been approved." I do not know if you're ready to delete the branch, etc., so feel free to squash and merge when you're ready.

@bikegeek bikegeek merged commit e5a0491 into main_v5.0 Feb 16, 2023
@bikegeek bikegeek deleted the feature_2016_update_python_reqs branch February 16, 2023 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Documentation: Update the METplus Components Python Requirements
2 participants