Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
PHEP 4: PyHC Package Tiering #31
base: main
Are you sure you want to change the base?
PHEP 4: PyHC Package Tiering #31
Changes from 1 commit
beb0c9e
4827407
10b6add
0ebfa4b
5f6a9a6
60620dd
c44670c
fa310db
367748b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe "Honorable Mention" should be renamed to "Copper" here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For ease of commenting, I would recommend breaking these lines at 79 characters. There are also minor grammar errors that would be easier to scrub with suggestions if the line wasn't so long.
Substantive comments:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, I'll modify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also suggest swapping the rows and columns for the tables to help reduce line size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I much prefer one line per sentence formatting for prose in git. git by default diffs per-line and a sentence should generally be a logical unit so if you change two parts of it a git conflict makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I'm a bit hesitant to include Interoperability Status in here, both because it's very hard to define and because our community is still working out what we mean when we say packages are interoperable.
✅ I like having PyHC ENv Installation Conflicts in here since it partially addresses the interoperability issue while being well-defined and impactful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely open to changing up the categories. Kind of threw in a "kitchen sink" thing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of linking this to the PyHC env. That would be easily testable. I would prefer not to have a definition of interoperability that requires a data object from one package be converted to a data object for another, since testing between each combination of core packages will grow rapidly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to remove the interoperability column, based on feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But keeping in the PyHC env conflicts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually silver is higher than bronze
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lack of sleep coding mistake. 🙃 Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend including links to pages that explain how to do each of these things, to make it easier for new people to fulfill each requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I'll add that/include in the "How to Teach This" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be standards, or PHEPs, or "standards and their replacements?"
As I noted in #33, #34, #35 it would be nice to replace our standards columns on the package page with PHEP compliance. More below.
Then instead of standards grades it would be more "compliance with standards-track PHEPs" and something like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the table approach so the requirements for each level are more specific and clearly laid out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am understanding correctly, in the end this would still be a kind of stoplight system like what we have now, but pointing to compliance to... PHEPs? I didn't see your issues before submitting my code here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, stoplight system but the columns are PHEPs instead of categories of standards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to go along with that. But, does that also mean this PHEP has to sit in limbo until those other complimentary PHEPs get passed? @jtniehof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I like leaving the "many should requirements" in the Gold-level packages. We really should have only the cream of the crop and those putting in the effort to fully comply requiring our highest tier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, put this in a related but different discussion thread, so now subjecting you to copy/paste:
I wouldn't think we need to hold this up until all standards PHEPs are done. If we have something like "when new standards-track PHEPs are approved packages have 6 (12?) months to self-evaluate and update their tiers", then we could in theory approve this with no new standards lined up. There's going to be a transition period regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this anything unique to PyHC, or just being in a pyOpenSci review? If the latter, can this link the appropriate pyOpenSci page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be specifically for the PyHC-pyOpenSci pairing review process (checking against both pyOpenSci reqs + PyHC-specific reqs). No link for that, yet. That's part of why I'm trying to get the community to chat about what we'd need to define "yes, fits in with the PyHC" during the pyOpenSci process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe explicitly say "to be defined in the future" or something? It feels a bit weird to be approving a standard that requires something that's not yet in place but I understand we can't do everything at once. Or this could be made more vague of "future collaborations" or something and the PHEP defining the pyOpenSci process would say "modifies PHEP 4 by adding...."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That kind of modification plan could work nicely. In that pathway, this PHEP would become the skeleton (most of) the other PHEPs would map to using a stoplight or yes/no system as appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some items, we can flesh it out here and not wait for another PHEP. Others will need this approach or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'm going to modify the wording a bit here based on this feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is very clear. What does a conflict mean here? I assume the only real way you get a conflict is that you require an old version of a package? Perhaps a better thing to say here would be a reference to #29 if that gets merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going to need some defining ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simply will remove it. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that I don't think any packages would be at a gold level (based on the current crieta in this PR), that would suggest no packages at a future summer school.
Maybe we should loosen this down to silver or bronze? Or maybe tweak the gold level requirements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. Happy to modify that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend having these be something people can opt in or out of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't even think of that, that's a good idea. I'll modify to indicate that that would be an optional perk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend dropping this from the table completely. I am sure there will be lots of smaller things like this that projects will or wont get pulled into. This feels very frivolous to include in a very formal specification document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Cadair I disagree that it's frivolous. As PyHC-Chat improves, inclusion within it could be a carrot—not the only, or biggest, but still a carrot—for people to work to move tiers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this seems counterintuitive to me. Shouldn't the packages that are less up-to-snuff receive more help? Perhaps a better way of splitting time would be having a pool of volunteers that could give time for things like code reviews and then receive code reviews from other people. That would be a nice way of creating more of a community environment in PyHC, but is not directly related to the tiers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. There's also an idea that packages who've done the work to be at a higher level get more help if there's extra funding for someone to poke around their code base (e.g. work through bug issues or solve long-standing open PRs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point! We should have community support mechanisms for packages to level up, as you suggest.
📌 I'm wondering if something like an annual unstructured Helio Hack Week (like Astro Hack Week) would provide good opportunities for us to support each other in this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a potential use case, having a path forward to help packages be pip/conda-installable would be a good support. A Hack Week could be useful here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would also be nice to just have a front page with a search bar that could let people find any project by name or by keyword.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We technically have this with @sapols 's updates to the Project page, but it could be made more clear, or placed in a more obvious location?