PDEP-1: Purpose and guidelines for pandas enhancement proposals #47444
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ main: | |
| - pandas_web.Preprocessors.blog_add_posts | ||
| - pandas_web.Preprocessors.maintainers_add_info | ||
| - pandas_web.Preprocessors.home_add_releases | ||
| - pandas_web.Preprocessors.roadmap_pdeps | ||
| markdown_extensions: | ||
| - toc | ||
| - tables | ||
|
|
@@ -157,3 +158,5 @@ sponsors: | |
| logo: /static/img/partners/r_studio.svg | ||
| kind: partner | ||
| description: "Wes McKinney" | ||
| roadmap: | ||
| pdeps_path: pdeps | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,110 @@ | ||||||
| # PDEP-1: Purpose and guidelines | ||||||
|
|
||||||
| - Date: 21 June 2022 | ||||||
|
There was a problem hiding this comment. Maybe "Created" instead of "Date"? Since this will typically the date when the process is started, and not when it was eg accepted, "created" might denote that more correctly (and both NEPs and PEPs seem to use that) |
||||||
| - Status: Accepted | ||||||
| - Discussion: [#47444](https://github.com/pandas-dev/pandas/pull/47444) | ||||||
| - Author: [Marc Garcia](https://github.com/datapythonista) | ||||||
|
|
||||||
| ## PDEP definition, purpose and scope | ||||||
|
|
||||||
| A PDEP (pandas enhancement proposal) is a proposal to a **major** change in | ||||||
|
There was a problem hiding this comment.
Suggested change
? (not fully sure, but "proposal to" sounds like it should be followed by a verb |
||||||
| pandas, in a similar way as a Python [PEP](https://peps.python.org/pep-0001/) | ||||||
| or a NumPy [NEP](https://numpy.org/neps/nep-0000.html). | ||||||
|
|
||||||
| Bug fixes and conceptually minor changes (e.g. adding a parameter to a function) | ||||||
| are out of the scope of PDEPs. A PDEP should be used for changes that are not | ||||||
| immediate and not obvious, and are expected to require a significant amount of | ||||||
| discussion and require detailed documentation before being implemented. | ||||||
|
|
||||||
| PDEP are appropriate for user facing changes, internal changes and organizational | ||||||
| discussions. Examples of topics worth a PDEP could include moving a module from | ||||||
| pandas to a separate repository, a refactoring of the pandas block manager or | ||||||
| a proposal of a new code of conduct. | ||||||
|
|
||||||
| ## PDEP guidelines | ||||||
|
|
||||||
| ### Target audience | ||||||
|
|
||||||
| A PDEP is a public document available to anyone, but the main stakeholders to | ||||||
| consider when writing a PDEP are: | ||||||
|
|
||||||
| - The core development team, who will have the final decision on whether a PDEP | ||||||
| is approved or not | ||||||
| - Developers of pandas and other related projects, and experienced users. Their | ||||||
|
There was a problem hiding this comment.
Suggested change
? (might be a bit more generic wording, as "developer" sounds more code-centric?) |
||||||
| feedback is highly encouraged and appreciated, to make sure all points of views | ||||||
| are taken into consideration | ||||||
| - The wider pandas community, in particular users, who may or may not have feedback | ||||||
| on the proposal, but should know and be able to understand the future direction of | ||||||
| the project | ||||||
|
|
||||||
| ### PDEP authors | ||||||
|
|
||||||
| Anyone can propose a PDEP, but in most cases developers of pandas itself and related | ||||||
| projects are expected to author PDEPs. If you are unsure if you should be opening | ||||||
| an issue or creating a PDEP, it's probably safe to start by | ||||||
| [opening an issue](https://github.com/pandas-dev/pandas/issues/new/choose), which can | ||||||
| be eventually moved to a PDEP. | ||||||
|
|
||||||
| ### Workflow | ||||||
|
|
||||||
| #### Submitting a PDEP | ||||||
|
|
||||||
| Proposing a PDEP is done by creating a PR adding a new file to `web/pdeps/accepted/`. | ||||||
| The file is a markdown file, you can use `web/pdeps/accepted/0001.md` as a reference | ||||||
|
There was a problem hiding this comment. I feel like it would be convenient to have an actual blank template PDEP file to be filled out. It might also be nice to have a script that generates the next PDEP number for you(I anticipate it might be hard to find the next PDEP number if a lot of PDEPs are submitted in the future). There was a problem hiding this comment. I'd personally leave that for later, once we need it. Those sound like good ideas to me, but I wouldn't implement them initially, I would wait to see how things work first. If we merge like PDEP per month, I think checking the last PDEP number before merging is easier than a system to autogenerate them. And for a template, I'd wait to have few actual PDEPs before deciding if it helps, or if PDEPs are too different from one to another. Does it make sense to you to start simple and iterate later as we have more experience? |
||||||
| for the expected format. | ||||||
|
|
||||||
| By default, we expect a PDEP will be accepted, so the PR of a PDEP should be done | ||||||
|
There was a problem hiding this comment. Do we want to also have a public comment period on PDEP's like tensorflow has with their RFC's? We should probably also clarify when voting happens(define what proportion of core team members need to consider a PDEP ready before voting like @mroeschke stated) and how long core team members have to vote. There was a problem hiding this comment. Happy to discuss and hear other opinions. But to me personally, I wouldn't have a voting period or deadline. I see PDEPs more like ongoing discussions, with feedback and updates, than just a voting process. Also, I assume tensorflow core devs are mainly google employees, so imposing a deadline makes more sense, as they are supposed to be working on the project X hours. But for a mostly volunteer developed project like pandas, I'd leave it more open. But again, I'd start by seeing how things work, and if we see PDEPs keep open for too long, and seems like having a timeframe should help, surely worth giving it a try. There was a problem hiding this comment.
I agree, just having a PDEP is akin to a new process, so having a process for the PDEP is maybe like introducing another meta process at the same time. So in response to this comment and others related to the process, I agree with @datapythonista that to begin with we just discuss whether we want to use PDEPs, what we want out of a PDEP, and roughly what it should contain and iterate on the gaps over time. The only caveat here, is that once someone has spent effort preparing a PDEP and it is approved that we don't have "blockers" implementing, so that does affect the approval process to some degree. There was a problem hiding this comment. i agree - we need a voting process here - we could emulate that of NEP which is pretty simple There was a problem hiding this comment.
I think what Marc proposed above is to defer a discussion about the decision process (basically our governance model) for a subsequent discussion. (which sounds good to me to keep the discussion manageable) There was a problem hiding this comment. i am not sure this is true - sure a lot will easily be accepted but some might be controversial and ultimately not accepted There was a problem hiding this comment. yeah. maybe just qualify this by changing before this text we have...
so the PEP is part of the detailed documentation and an issue should perhaps be the initial part of the discussion. (just as I would normally expect say a bug fix PR to have an associated issue) and we also have
I think that maybe issues should always be opened before submitting a PEP, either as a specific issue or an existing issue concluding that a PEP is required. Maybe we also need to ensure that If we have some form of gate for opening a PEP, then most should be accepted. There was a problem hiding this comment. We could also just leave out the "By default, we expect a PDEP will be accepted" ? To me it doesn't seem to add much, rather than a potentially wrong/confusing message (in general PDEPs are for topics that are not trivial and thus will not always be accepted) |
||||||
| in the `accepted` directory and contain `Status: Accepted`. If a PDEP is finally | ||||||
|
There was a problem hiding this comment. We could alsof default to start a PDEP in a "draft" status (or "under discussion", I see there is a section for those)? There was a problem hiding this comment. Sounds good to me. Regarding merging PDEPs as draft, feels like it could make discussions more difficult. Like where to have the discussion once the PR is merged. Via new PRs? Or in a second PR, not being able to comment in parts not in the diff. But I guess in some cases it can be useful, maybe a PR with different parts, and merging as a Draft once the first part has been discussed, and before working on other parts. I'll update the document if there are no objections to using Under discussion status (or Draft if people prefer this name) until PR is Accepted. And to consider merging PRs before they are accepted when it's useful. There was a problem hiding this comment. IMO I think the current guidance stated here is clear as long as a PDEP is an open PR, it should be considered "under discussion" and a draft PDEP can be a draft PR for example. I agree with @datapythonista that merging intermediate states of a PDEP would fragment the discussion and there shouldn't be more than one page/PR/location where a PDEP can be discussed. There was a problem hiding this comment.
We will probably have to see how it goes in practice. For many PDEPs a single PR with proposal, discussion, acceptance all in one will quite likely be sufficient. As some references: numpy seems to already merge NEP PRs if they are still under discussion (eg numpy/numpy#18456), but they also have substantial discussion on the mailing list. Similarly in Python, PEPs are merged quickly, but also there the discussion is mostly happening on mailing list or discourse. That's of course different with the current way in pandas where most discussion typically happens on GitHub. |
||||||
| rejected, its status and directory will be updated by the core team before merging, | ||||||
| once the decision is made. Please make sure you select the option | ||||||
| `Allow edits and access to secrets by maintainers` when opening the PR. | ||||||
|
|
||||||
| #### Accepted PDEP | ||||||
|
|
||||||
| A PDEP will be accepted by the core development team, and decisions will be made | ||||||
|
There was a problem hiding this comment. Maybe reword "will" to something else? This language makes it feel as if all PDEP's will be accepted. |
||||||
| based on the [pandas governance document](https://github.com/pandas-dev/pandas-governance/blob/master/governance.md). | ||||||
|
|
||||||
| Once a PDEP is accepted, any contributions can be made toward the implementing the PDEP with an open-ended completion timeline . The | ||||||
| pandas project development, with a mix of volunteers and developers paid from | ||||||
| different sources, and development priorities are difficult to understand or | ||||||
| forecast. For companies, institutions or individuals with interest in seeing a | ||||||
| PDEP being implemented, or to in general see progress to the pandas roadmap, | ||||||
| please check how you can help in the [contributing page](/contribute.html). | ||||||
|
|
||||||
| #### Implemented PDEP | ||||||
|
|
||||||
| Once a PDEP is implemented and available in the main branch of pandas, its | ||||||
mroeschke marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| status will be changed to implemented, so there is visibility that the PDEP | ||||||
| is not part of the roadmap and future plans, but a change that it already | ||||||
| happened. The first pandas version in which the PDEP implementation is | ||||||
| available will also be included in the PDEP. | ||||||
|
|
||||||
| #### Rejected PDEP | ||||||
|
|
||||||
| A PDEP can be rejected when the final decision is that its implementation is | ||||||
| not the best for the interests of the project. They are as useful as accepted | ||||||
|
There was a problem hiding this comment. The word "accepted" seems a bit strange here, since this is about PDEPs that are not accepted? There was a problem hiding this comment. This is to contrast rejected PDEPs against accepted PDEPs; i.e. "even rejected PDEPs are useful". I think the wording is okay, but maybe "Rejected PDEPs are just as useful as accepted PDEPs..." would make it more clear? There was a problem hiding this comment. Ah, I missed the first "as" in the sentence, so I read it as "they are useful as accepted PDEP", instead of "they are as useful as accepted PDEPs", hence the confusion. |
||||||
| PDEPs, since there are discussions that are worth having, and decisions about | ||||||
| changes to pandas being made. They will be merged with `Status: Rejected`, so | ||||||
| there is visibility on what was discussed and what was the outcome of the | ||||||
| discussion. A PDEP can be rejected for different reasons, for example good ideas | ||||||
| that aren't backward-compatible, and the breaking changes aren't considered worth | ||||||
| implementing. | ||||||
|
|
||||||
| #### Invalid PDEP | ||||||
|
|
||||||
| For submitted PDEPs that do not contain proper documentation, are out of scope, or | ||||||
| are not useful to the community for any other reason, the PR will be closed after | ||||||
| discussion with the author, instead of merging them as rejected. This is to not | ||||||
| add noise to the list of rejected PDEPs, which should contain documentation as | ||||||
| good as an accepted PDEP, but where the final decision was to not implement the changes. | ||||||
|
|
||||||
| ## Evolution of this PDEP | ||||||
|
|
||||||
| While most PDEPs aren't expected to change after accepted, in some cases like this | ||||||
| PDEP, they will be updated to contain its latest version, if things evolve. A log | ||||||
|
There was a problem hiding this comment. We should probably clarify when a PDEP can be revised v.s. when a new PDEP has to be submitted superseding the old PDEP. |
||||||
| of the summary of the changes will be kept to make it easier to see if any change | ||||||
| has happened. | ||||||
|
|
||||||
| ### PDEP-1 History | ||||||
|
|
||||||
| - 21 June 2022: Initial version | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |
| import importlib | ||
| import operator | ||
| import os | ||
| import pathlib | ||
| import re | ||
| import shutil | ||
| import sys | ||
|
|
@@ -185,6 +186,59 @@ def home_add_releases(context): | |
| ) | ||
| return context | ||
|
|
||
| @staticmethod | ||
| def roadmap_pdeps(context): | ||
| """ | ||
| PDEP's (pandas enhancement proposals) are not part of the bar | ||
| navigation. They are included as lists in the "Roadmap" page | ||
| and linked from there. This preprocessor obtains the list of | ||
| PDEP's in different status from the directory tree and GitHub. | ||
| """ | ||
| context["pdeps"] = { | ||
| "accepted": [], | ||
| "rejected": [], | ||
| "under_discussion": [], | ||
| "implemented": [], | ||
| } | ||
| # accepted, rejected and implemented | ||
| pdeps_path = ( | ||
| pathlib.Path(context["source_path"]) / context["roadmap"]["pdeps_path"] | ||
| ) | ||
| for status in ("accepted", "rejected", "implemented"): | ||
| status_dir = pdeps_path / status | ||
| if not status_dir.is_dir(): | ||
| continue | ||
| for pdep in status_dir.iterdir(): | ||
| if pdep.suffix != ".md": | ||
| continue | ||
| html_file = pdep.with_suffix(".html").name | ||
| print(pdep) | ||
| with pdep.open() as f: | ||
| title = f.readline()[2:] # removing markdown title "# " | ||
| context["pdeps"][status].append( | ||
| { | ||
| "title": title, | ||
| "url": f"/pdeps/{status}/{html_file}", | ||
| } | ||
| ) | ||
|
|
||
| # under discussion | ||
| github_repo_url = context["main"]["github_repo_url"] | ||
| resp = requests.get( | ||
| "https://api.github.com/search/issues?" | ||
| f"q=is:pr is:open label:PDEP repo:{github_repo_url}" | ||
| ) | ||
| if context["ignore_io_errors"] and resp.status_code == 403: | ||
| return context | ||
| resp.raise_for_status() | ||
|
|
||
| for pdep in resp.json()["items"]: | ||
| context["pdeps"]["under_discussion"].append( | ||
| {"title": pdep["title"], "url": pdep["url"]} | ||
| ) | ||
|
|
||
| return context | ||
|
|
||
|
|
||
| def get_callable(obj_as_str: str) -> object: | ||
| """ | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.