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

Documentation: Review and update Requirements #1978

Closed
8 of 22 tasks
jprestop opened this issue Dec 6, 2022 · 5 comments · Fixed by #2177
Closed
8 of 22 tasks

Documentation: Review and update Requirements #1978

jprestop opened this issue Dec 6, 2022 · 5 comments · Fixed by #2177
Assignees
Labels
component: documentation Documentation issue priority: high High Priority reporting: DTC NCAR Base NCAR Base DTC Project reporting: DTC NOAA BASE NOAA Office of Atmospheric Research DTC Project requestor: NOAA/EMC NOAA Environmental Modeling Center required: FOR OFFICIAL RELEASE Required to be completed in the official release for the assigned milestone type: task An actionable item of work
Milestone

Comments

@jprestop
Copy link
Collaborator

jprestop commented Dec 6, 2022

Describe the Task

Review and update the METplus Components Python Requirements Documentation and the requirements.txt file for the upcoming release to ensure that they are current for the release documentation. These will likely be used by NCO for WCOSS2 installations.

Time Estimate

<1 day

Sub-Issues

Consider breaking the task down into sub-issues.

  • Add a checkbox for each sub-issue here.

Relevant Deadlines

METplus-5.0.0 release

Funding Source

Split across 2700042, 2702691, 2792542

Define the Metadata

Assignee

  • Select engineer(s) or no engineer required
  • Select scientist(s) or no scientist required

Labels

  • Select component(s)
  • Select priority
  • Select requestor(s)

Projects and Milestone

  • Select Repository and/or Organization level Project(s) or add alert: NEED PROJECT ASSIGNMENT label
  • Select Milestone as the next official version or Future Versions

Define Related Issue(s)

Consider the impact to the other METplus components.

Task Checklist

See the METplus Workflow for details.

  • Complete the issue definition above, including the Time Estimate and Funding Source.
  • Fork this repository or create a branch of develop.
    Branch name: feature_<Issue Number>_<Description>
  • Complete the development and test your changes.
  • Add/update log messages for easier debugging.
  • Add/update unit tests.
  • Add/update documentation.
  • Add any new Python packages to the METplus Components Python Requirements table.
  • Push local changes to GitHub.
  • Submit a pull request to merge into develop.
    Pull request: feature <Issue Number> <Description>
  • Define the pull request metadata, as permissions allow.
    Select: Reviewer(s) and Linked issues
    Select: Repository level development cycle Project for the next official release
    Select: Milestone as the next official version
  • Iterate until the reviewer(s) accept and merge your changes.
  • Delete your fork or branch.
  • Close this issue.
@jprestop jprestop added component: documentation Documentation issue priority: high High Priority requestor: NOAA/EMC NOAA Environmental Modeling Center type: task An actionable item of work alert: NEED ACCOUNT KEY Need to assign an account key to this issue required: FOR OFFICIAL RELEASE Required to be completed in the official release for the assigned milestone labels Dec 6, 2022
@jprestop jprestop added this to the METplus-5.0.0 milestone Dec 6, 2022
@georgemccabe
Copy link
Collaborator

@jprestop , the requirements.txt file was updated with PR #1958. The version of Python was also updated to 3.8.6 in the Requirements Documentation in the same PR.

I'm not sure how to determine if the version requirements for the packages have changed for any of the use cases or other METplus components. The minimum METplus wrappers requirements are accurate (Python 3.8.6 and python-dateutil). I would assume that the minimum versions listed in the doc are still accurate as the minimum version needed to run those cases. Some of the packages have higher versions in the Python 3.8.6 Conda environments that we use for testing than what is shown in this table, but I don't think that updating them to match is correct because a lower version could still work and we don't want to list higher requirements than necessary.

Let me know how you think I should proceed.

@georgemccabe
Copy link
Collaborator

georgemccabe commented Dec 6, 2022

@jprestop, Regarding the Requirements Documentation:

I find it very misleading that many of the Python packages listed here note "METplus wrappers" as one of the METplus Components that require it, but the minimum requirements for running the METplus wrappers is Python 3.8.6+ and dateutil. All of the other packages are only required for specific use cases. I think it should be more clearly noted that this is the case. @JohnHalleyGotway suggested that we could have check boxes for METplus Wrappers, Specific Use Cases, METplotpy, and METcalcpy, then check the box for each that is relevant. Also, moving dateutil to the top just below Python 3.8.6 would make it more clear that those are the actual minimum requirements of the wrappers.

I also find the table very hard to navigate since you have to go to the bottom of the entire table to scroll over to the right to see the rest of the content. The content that is blocked by the grey area on the right is actually missing from the PDF version of the docs (see page 4 and 5). Perhaps there is a way to remove the width restriction to show the whole table if the window is large enough? This way the scroll bar at the bottom of the browser could be used.

@jprestop
Copy link
Collaborator Author

jprestop commented Dec 9, 2022

@georgemccabe

the requirements.txt file was updated with PR #1958. The version of Python was also updated to 3.8.6 in the Requirements Documentation in the same PR.

Awesome! Thank you! Based on how small that file is:

certifi==2022.12.7
python-dateutil==2.8.2
six==1.16.0

I assume those must be the minimum requirements and not what is needed to run all of the METplus use cases. It sounds like NCO would like to have a requirements.txt file (at least for METcalcpy, METplotpy, and METdataio, which are not yet established on WCOSS2) that they could use to install everything the software needs. They would probably like that have that for METplus as well, although, I know EMC users are using more than just the minimal requirements. I'm guessing most of the packages for METplus that EMC needs are already installed, however, there could be others that are needed. I don't know offhand.

I'm not sure how to determine if the version requirements for the packages have changed for any of the use cases or other METplus components.

Ideally, this checkbox in the Issue template would catch any new changes:

Add any new Python packages to the [METplus Components Python Requirements](https://metplus.readthedocs.io/en/develop/Users_Guide/overview.html#metplus-components-python-requirements) table.

as would this checkbox in the PR template would catch any new changes:

Add any new Python packages to the [METplus Components Python Requirements](https://metplus.readthedocs.io/en/develop/Users_Guide/overview.html#metplus-components-python-requirements) table.

The minimum METplus wrappers requirements are accurate (Python 3.8.6 and python-dateutil).

What are these used for and are they not considered minimum requirements?

certifi==2022.12.7
python-dateutil==2.8.2
six==1.16.0

Let me know how you think I should proceed.

Since NCO didn't directly ask for this for METplus, it's probably fine to not worry about now. I was just being proactive since they said they wanted it for the METplus-Analysis tools.

I find it very misleading that many of the Python packages listed here note "METplus wrappers" as one of the METplus Components that require it, but the minimum requirements for running the METplus wrappers is Python 3.8.6+ and dateutil. All of the other packages are only required for specific use cases. I think it should be more clearly noted that this is the case.

I'm sorry that you feel that way. I think the column titled "Use Cases (only applicable for METplus wrappers component)(followed by python package name)" was intended to indicate that the package listed is required for functionality of those specific use cases.

@JohnHalleyGotway suggested that we could have check boxes for METplus Wrappers, Specific Use Cases, METplotpy, and METcalcpy, then check the box for each that is relevant.

Checkboxes within the column "METplus Component" or is this suggesting a different structure entirely?

Also, moving dateutil to the top just below Python 3.8.6 would make it more clear that those are the actual minimum requirements of the wrappers.

I think they were in alphabetical order for convenience in locating.

I also find the table very hard to navigate since you have to go to the bottom of the entire table to scroll over to the right to see the rest of the content. The content that is blocked by the grey area on the right is actually missing from the PDF version of the docs (see page 4 and 5). Perhaps there is a way to remove the width restriction to show the whole table if the window is large enough? This way the scroll bar at the bottom of the browser could be used.

Yes, these big tables are difficult. Thank you for pointing out that they are cut off in the PDF. We must have quite a few places in the documentation where that is happening, unfortunately. Perhaps there is some way to remove the width restriction - we'd have to look into it.

@georgemccabe
Copy link
Collaborator

@jprestop, I generated the requirements.txt file by creating a fresh Python install, then installed the python-dateutil package. The certifi and six also showed up when I ran pip freeze. I'm not sure if those show up when you run pip freeze on a fresh Python install or if they were added as dependencies to python-dateutil. Either way, I don't think they are necessary to include in the documentation because they aren't explicitly installed.

Regarding the suggested reformatting of the python package table, we didn't discuss exactly how it would be formatted, but just noted that it is confusing and how it could potentially be improved.

Do I need to take any other action on this task? Or can it be closed or moved to another development cycle? I am ready to cut the METplus 5.0.0 release as soon as MET is ready, but I wanted to make sure there isn't anything else needed to complete this issue?

@jprestop
Copy link
Collaborator Author

Hi @georgemccabe. Thank you for your slack message bringing my attention to this issue. My apologies for missing your message from so long ago. I just updated the docs/Users_Guide/overview.txt file for the upcoming 5.1.0 release and will submit a PR for your review.

@jprestop jprestop assigned jprestop and unassigned georgemccabe May 17, 2023
@jprestop jprestop linked a pull request May 17, 2023 that will close this issue
14 tasks
@georgemccabe georgemccabe changed the title Review and update Requirements Documentation Documentation: Review and update Requirements May 17, 2023
@jprestop jprestop added reporting: DTC NCAR Base NCAR Base DTC Project reporting: DTC NOAA BASE NOAA Office of Atmospheric Research DTC Project and removed alert: NEED ACCOUNT KEY Need to assign an account key to this issue labels May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: documentation Documentation issue priority: high High Priority reporting: DTC NCAR Base NCAR Base DTC Project reporting: DTC NOAA BASE NOAA Office of Atmospheric Research DTC Project requestor: NOAA/EMC NOAA Environmental Modeling Center required: FOR OFFICIAL RELEASE Required to be completed in the official release for the assigned milestone type: task An actionable item of work
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants