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

Should we require a separate fpm package first before standardizing? #658

Open
certik opened this issue May 18, 2022 · 18 comments
Open

Should we require a separate fpm package first before standardizing? #658

certik opened this issue May 18, 2022 · 18 comments
Labels
meta Related to this repository

Comments

@certik
Copy link
Member

certik commented May 18, 2022

One thing that I am worried is that we are making the same mistake as the standards committee, often standardizing things before we get actual experience with using it. Now with fpm, I wonder if it's time to first require creating an fpm package with the addition, and get people using it, before standardizing it in stdlib?

Take #580 as an example. It's been open for half a year, but how many people have actually tried it in their projects? I didn't, because it's not that easy to test a PR, you have to clone the repository, checkout the PR, install stdlib by hand, etc. You can't use it via fpm (as far as I know, since fpm requires a special branch, that is not generated from PRs), we could fix that by committing all the fypp generated files into git, then we could update fpm to support depending on PRs. However, even that seems relatively fragile and temporary.

Instead, if #580 was posted in a standalone git repository as a regular package, then people can immediately use it in their projects using fpm. If it later gets standardized, then it's a simple change of an fpm dependency, and possibly of a use line. And there is no rush with this change either. People could contribute patches to the repository, tests, etc.

An example would be the argparse package in the Python standard library, which first lived as a standalone package, well liked, and then later standardized.

@certik certik added the meta Related to this repository label May 18, 2022
@certik
Copy link
Member Author

certik commented May 18, 2022

Note that argparse was the second attempt at argument parsing in Python. The first module was optparse, but it was too limiting, so a lot of people ended up using the argparse separate module instead.

@14NGiestas
Copy link
Member

About not being able to use fpm to test PR, you actually can but it's just undocumented, you just need to run the ci/fpm_deployment.sh it will generate a folder called stdlib-fpm (which will be the content of the homonymous branch).
Is somewhat annoying though but it's low effort:

  • You first need to clone the forked repo (that could take a while in a slow connection)
  • Then you git switch <PR-branch-name> (finding the right PR branch can be a minor annoyance)
  • And finally, you run the ./ci/fpm_deployment.sh to build the folder
  • cd stdlib-fpm
  • fpm build, fpm test, or whatever
    I usually create an app/main.f90 where I use the implemented stuff to poke around the PR, usually pulling stuff from docs/specs and tests.

I wonder if the CI can create something like a "release" when a PR happens (in its fork of course), so you just download the fpm package, unpack it, and test.

Or maybe just documenting the process above in a PR template (which I'm extrapolating, does it exists?)

@urbanjost
Copy link

urbanjost commented May 19, 2022

I totally support some kind of fpm-based development of stdlib features,
and have suggested people actually try the available date-time packages,
ANSI color packages, .... and from that make suggestions on what should
be adapted in stdlib.

I was hoping something like a central wiki for functions such as these
would be part of the stdlib development, where people could add their
packages for consideration for a certain functionality, and that people
could try them and make suggestions based on this for what the stdlib
implementation should be, and/or that a clear favorite would emerge from
the candidates.

That is exactly what I tried to create for this very topic with
M_escape and subsequently
M_attr. Didn't seem to get
much traction.

Perhaps something like a sponsored set of moderated categories based off
the Fortran-lang pages would work instead? Discussions could proceed
using current methods, but the Fortran-lang pages would keep a simple
digest of the materials, like a list of the candidates and a list of
the desired functionality? Not sure if moderators are available for
such an approach. Somewhat like a competitive Rosetta Code? It
could start with a description of a general functionality, and peoples
ideas about approaches
...

For this topic, maybe the model suggestions would have looked like

  • basic terminal control sequence constants
  • a functional model
  • using an XML-like syntax for attributes like color
  • extending type(string) to have attributes and adding a DT for printing it
  • writing latex/html/pdf (see asa2pdf in GPF)
  • A preprocessor for an extended Fortran Format to be proposed as a Fortran
    feature (ie. write(*,"C.1.4.U,A,C)")'error' would use the C descriptor to add
    foreground color 1, background color 4, and underlining, and "C" would reset).
  • a new full-blown Fortran object-oriented ncurses-like library
  • an ISO_C_BINDING interface to termlib/terminfo functionality in existing C
    libraries
  • a proposal for attributes like color to be added to the common extensions
    for supportng ASA output, combined with a proposal for the support to become
    part of the standard
  • etc ...

and functionality could be listed and voted on by people actually indicating
what they would use or how critical a functionality would be, like

  • full color support versus eight basic colors
  • string positioning
  • oop design versus functional
  • would overloading of operators be a plus or minus?
  • ability to turn on and off
  • etc ...

people could then submit their fpm packages for consideration and discussion?

So basically, I think fpm is a near-perfect tool for creating various
prototypes for stdlib functionality, but by itself has not become a path for
such.

What is missing that this is not the case? Would a "request for papers"
approach work, with a wiki used for listing desired features, describing
current roadblocks, and listing current related packages work?

Is there some model other languages use that would be useful?

@awvwgk
Copy link
Member

awvwgk commented May 19, 2022

It's been open for half a year, but how many people have actually tried it in their projects?

Unfortunately, this is currently the default for most PRs at stdlib taking several weeks to months until they get reviewer attention and not a criteria to judge a PR.

Regarding fpm packages for PRs, I'm happy to support this idea and have done so in the past, especially for strings. From my experience with those was not a big difference in review time nor adaption so far whether the project existed as an fpm package before.

On the other hand, creating, maintaining and establishing an fpm package is a lot of hard work, especially maintaining the CI testing, automated documentation and maybe the fypp processing puts a huge demand on the contributors. At least for me pushing a project on GitHub means a significant time commitment for maintenance, working with reviewers on a PR of course requires also effort, however this is usually only coding rather than building reliable and long-lived software infrastructure.

So while I'm in principle in favor of this idea, I worry it might hamper the pace of developing stdlib even further.

@milancurcic
Copy link
Member

milancurcic commented May 19, 2022

I overall share @awvwgk's opinion. It seems a good idea at a surface, but I don't think it would help in practice.

One thing that I am worried is that we are making the same mistake as the standards committee, often standardizing things before we get actual experience with using it.

I think this is an incorrect premise to start with. The problem with the Fortran standard and the committee is that there are features that are insufficiently tested and/or well-thought-through, and the committee requires backward compatibility in most cases (understandably so).

This is not the case with stdlib. As we discussed many times before, we have an experimental level and stable tiers of features. Anything in experimental (currently everything) is subject to change. Features in stable are really what's comparable to standardized language features.

Take #580 as an example. It's been open for half a year, but how many people have actually tried it in their projects? I didn't, because it's not that easy to test a PR, you have to clone the repository, checkout the PR, install stdlib by hand, etc. You can't use it via fpm

For example, I at least play a little bit for any feature PR that I review, unless I indicate in the review that I didn't try it but seems reasonable. In my view, it's really when features are committed in experimental that the real playing and testing begins, not the PR stage. I think we should be fast and loose with feature PRs to experimental (Is it in scope? Yes. Does it seem useful? Yes. Is something obviously incorrect about it? No. OK, let's merge it.), and only very conservative at the stage when we consider features for the stable tier. Those will obviously need to show a track record of use in other packages.

So, if I'd recommend any change, it's for people to review, merge sooner, don't wait for approval from many people, and iterate in short cycles. If we realize later that an API or an implementation should be improved, we can do it.

@certik
Copy link
Member Author

certik commented May 19, 2022

Yes, it's all about reducing the burden on both stdlib maintainers, and stdlib users and reviewers (often times but not always the same people).

I can see only two ways forward:

  • The fpm approach I posted above
  • Merge PRs relatively quickly to experimental, and use stdlib itself as the test project for people to use.

Right now we do not do either, and that is what I object against. I am fine with either one approach, and Milan is right that we have discussed initially to use experimental precisely as a test project to make it easy for people to try out and use, and we reserve full right to completely change the API or even remove the functionality.

So let's get back to our original plan, and merge much quicker to stdlib.

The other part of the problem is that stdlib seems to be missing a single person who would be in charge of this, and ensure PRs get reviewed in a timely manner and merged and actively pushing after the goal/vision of stdlib. I can't be that person, as I am already that person for LFortran. Is there anybody who would be interested for this role? @awvwgk, @milancurcic, @14NGiestas, @urbanjost and others?

@milancurcic
Copy link
Member

I'm at capacity and do my best. Yes, ideally there should be a single person that's consistently on top of it, but it may be difficult to find. Until we do, perhaps we just need to broaden the team of "partial maintainers".

@milancurcic
Copy link
Member

Currently, stdlib PRs require approval from 2 maintainers (members with write access) for it to be merged. This number of 2 was arbitrary when I set it a long time ago. And nowadays we often get approval from reviewers without write-access, which doesn't count toward the required approval quota.

Should we lower this requirement to 1 approval, with the hope of PRs getting merged faster?

@certik
Copy link
Member Author

certik commented May 19, 2022

Yes, a single person in charge is the best. Until we can find such a person, then let's do a collection of people as partial maintainers (as we do now) and let's do our best not to delay things with "indecision". (I realize my objections partly caused the delay, but until we have a single person to push for this, please don't get discouraged if I am critical. Merging faster is a higher priority for me than my milder objections, which I still think was important to write.)

@jvdp1
Copy link
Member

jvdp1 commented May 19, 2022

The main issues for stdlib is the lack of reviewers. I try to review most of them, but I am currently lacking time, and the PR are usually quite large.

My main concern is that if we merge quickly feature PRs after a "light" review, these feature PRs that might be half backed and full of (hidden) issues, will remain as they are in stdlib, because users will used them without reporting issues or providing improvements, or will not use them at all. Why would reviewers appear after the PR are merged, while they are lacking when the PR are opened?

Currently I already integrated a few features (e.g. logger, some stat functions, sorting procedures) in my main projects, and use daily almost all features through fpm. I was quite involved in reviewing the logger and sorting procedures, and if they would have been merged in their first submission, I can affirm that I would not use them today because they did not fit my needs in their original proposition, AND I would not try to improve them (or submit issues/PR).

Therefore, while slow, I prefer to have well though and backed features, even in the experimental space.

Lowering the number of approvals for merging could be also a problem. For example, there is currently a PR with one approval, but the specs and the tests do not fully agree with the stdlib requests/defaults. Merging it as is would result in a feature with not-conforming/"poor" specs and tests, which might result in bad reputation for stdlib.

Note that I am not believing in having a new feature has a separate project. It is already the case for the stdlib_os project, and little tests and improvements have been observed.

Finally, I believe that the best thing would be to have fpm that can build stdlib from scratch, without the need of the deployement script for fpm. If it would be the case, anyone could build stdlib from any branch of any fork and review and test it appropriately. This would help me a lot when reviewing PRs.

@ivan-pi
Copy link
Member

ivan-pi commented May 19, 2022

Personally I think 2 approvals is not the problematic point, but indeed finding the time for reviews, or ideally new reviewers. Maybe once a PR reaches a certain level of maturity, submitting a description and demonstration to Discourse could raise more interest.t

I'm hesitant to make submitting a fpm package a requirement. It could be a recommendation, and I know @awvwgk has done this many times (also in issues), but it requires a lot of effort, and I fear would raise the bar too high for new contributors.

@certik
Copy link
Member Author

certik commented May 19, 2022

@jvdp1 I share your concerns, and that's why I opened up this issue. I think you are saying not to merge too quickly to the experimental part of stdlib. You are also saying not to use separate fpm projects.

As such, the third way forward that I think you are suggesting is to make it so that fpm can easily build any branch / PR right away (whether by committing the fypp generated codes into git or by teaching fpm about fypp so it can automatically run it). Then we can keep the PRs open for longer.

@awvwgk
Copy link
Member

awvwgk commented May 19, 2022

There is fortran-lang/fpm#78 open since a while, it will take a reasonable effort to integrate the preprocessing stage in the current target generation structure. It's a small to medium-sized project and will require some time commitment mind you, but the fpm maintainers are of course around to provide pointers if needed.

If anyone here feels deeply about building stdlib with fpm without running the deploy script locally first, this is the right place to put in some energy and effort.

@jvdp1
Copy link
Member

jvdp1 commented May 19, 2022

Then we can keep the PRs open for longer.

@certik You summarized nicely my message. Thank you. However, I hope that compiling any branch with fpm would result in merging faster the PRs, as the users could test quicker the proposed features in their projects.

@14NGiestas
Copy link
Member

The other part of the problem is that stdlib seems to be missing a single person who would be in charge of this, and ensure PRs get reviewed in a timely manner and merged and actively pushing after the goal/vision of stdlib. I can't be that person, as I am already that person for LFortran. Is there anybody who would be interested for this role? @awvwgk, @milancurcic, @14NGiestas, @urbanjost and others?

I'll keep trying to push some PRs/issues forward since I have time to spare now (but I don't know how long I can consistently do it or even if it's ok - please let me know).

The main issues for stdlib is the lack of reviewers. I try to review most of them, but I am currently lacking time, and the PR are usually quite large.

I'm not sure if it's the case, but one thing that may help is to ask (encourage) everyone to do some light reviews even if you are not 100% familiar with the PR topic, the more people looking at the code the better (especially in huge PRs).
I don't think anyone should be an expert to catch odd stuff in other people's code and with the help of the PR contributor, we can figure out what is going on. This way we can avoid waiting too much for someone to feel comfortable / find time to extensively review a PR and even relieve the burden on both sides (reviewers and contributor).

@jalvesz
Copy link
Contributor

jalvesz commented Jan 7, 2024

After reading some of the comments here I feel that another pain point is "how easy is it to get someone's PR locally to actually test it and review it"? Which is not necessarily an easy task. Skimming around I found this post https://andrewlock.net/working-on-two-git-branches-at-once-with-git-worktree/ that addresses precisely this by showing how to use git worktree. Maybe this could be useful for improving PR reviews?

@jvdp1
Copy link
Member

jvdp1 commented Jan 7, 2024

Skimming around I found this post https://andrewlock.net/working-on-two-git-branches-at-once-with-git-worktree/ that addresses precisely this by showing how to use git worktree.

I didn't know git worktree. I'll give it a try. Thank you.

Maybe this could be useful for improving PR reviews?

Maybe. However, I think that the best would be that fpm supports fypp natively. Maybe we should first focus on that task first.

@jalvesz
Copy link
Contributor

jalvesz commented Jan 9, 2024

Maybe. However, I think that the best would be that fpm supports fypp natively. Maybe we should first focus on that task first.

Ok, so I think I understood where my confusion was, and I see that there are two issues:

  • I did not notice that there was a dedicated branch stdlib-fpm for just building,... So, when I got here, I just saw the main branch and my reflex was: fork > work on my main with the extra step of pre-processing using the script > try to PR. Here I think that indeed, it is necessary to enable fpm preprocessing the .fypp, BUT it should not be hidden that for the moment this requires 1 extra step. I would discourage anyone to try to contribute by first going trough the stdlib-fpm branch as it hides that the actual implementation can be more generic with fypp, and it slows down the whole contribution process as @certik mentioned ... rather try to better explain the whole process.
  • Working locally on different PRs: besides git worktree, another git option that could help is signaled here https://stackoverflow.com/questions/1384325/in-git-is-there-a-simple-way-of-introducing-an-unrelated-branch-to-a-repository/4288660#4288660 git checkout --orphan newbranch

Just slightly disconnected, https://fortran-lang.discourse.group/t/what-kind-of-tests-are-sufficient-some-personal-thoughts/7137/7 the video linked here is honesty a gem regarding building up the ecosystem, it is worth's listening to the whole talk :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Related to this repository
8 participants