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

Updating edge futures -> Promises (preparation for later merge) #522

Merged
merged 68 commits into from
Dec 27, 2016

Conversation

pitr-ch
Copy link
Member

@pitr-ch pitr-ch commented Mar 14, 2016

  • yard documentation
  • evaluator of Ruby examples in markdown files
    • move out to its own gem
  • resolve naming
  • guide like documentation
  • move cancelation and throttle out of the promises file

Not required for merge:

  • polish cancellation
  • polish throttle

@pitr-ch pitr-ch added the enhancement Adding features, adding tests, improving documentation. label Mar 14, 2016
@pitr-ch pitr-ch self-assigned this Mar 14, 2016
@pitr-ch pitr-ch added this to the 1.1.0 milestone Mar 14, 2016
@jdantonio
Copy link
Member

I was just about to respond to you on the other thread when I received the notification of this pull request. I think it is far too early for this. I have a number of questions I was going to ask in the other thread (most notably, "why?") and there are many things that still need discussed (most notably the API, which I've said before I think needs more discussion).

Rails 5 is still in Beta. I don't think it's prudent for us to do anything more than make bug fixes to core at this time.

I'll send you an email and we can discuss this in more depth offline.

@pitr-ch
Copy link
Member Author

pitr-ch commented Mar 14, 2016

Ok, looking forward for you email.

@pitr-ch pitr-ch changed the title Merging new futures Updating edge futures -> Promises (preparation for later merge) Jun 13, 2016
@pitr-ch pitr-ch modified the milestones: 1.0.3, 1.1.0 Jun 13, 2016
@pitr-ch pitr-ch force-pushed the futures branch 2 times, most recently from 3d42ba1 to a66565e Compare June 13, 2016 22:10
@pitr-ch
Copy link
Member Author

pitr-ch commented Jun 13, 2016

Lots of updates. Will require major bump of edge.

@ianks
Copy link
Contributor

ianks commented Jun 14, 2016

@jdantonio, what is the motivation for having rails dictate the cadence of the project? I think CR has great benefits outside of the Rails ecosystem and should be treated as independent from Rails development.

@iNecas
Copy link
Contributor

iNecas commented Jun 14, 2016

@pitr-ch any backward incompatible changes you are aware of?

@pitr-ch
Copy link
Member Author

pitr-ch commented Jun 14, 2016

@iNecas only surface changes to avoid any more changes when it eventually goes to core: new namespace, some aliases gone, few renames for consistency etc. No big internal changes.

@jdantonio
Copy link
Member

Rails is not dictating the cadence of this project. I am dictating the cadence of this project. And that cadence is slow and steady.

This gem has been downloaded almost 5 million times. The latest release has been downloaded 800,000 times. Those numbers will grow much higher once Rails 5 is released and teams begin upgrading. This is a huge accomplishment and something we should all be proud of. Very few open source projects ever reach this level of adoption. It also means that we have different responsibilities now.

There is no value in rushing headlong into a 2.0 release that breaks nearly every major API, especially when none of our users are asking for this change. Our users are happy, productive, and we are making a difference within the Ruby community. We must protect the trust we have been given. We will not repeat the mistakes of Angular. Instead, we will follow the example of Ember. We will slowly, deliberately, and with measured pace integrate features from edge into core, simultaneously deprecating features in 1.0. Once we have everything new into core we will release a 2.0 version that simply removes all deprecated features.

We will not, however, merge all edge features as-in into core. There are numerous changes which need made. As with 1.0, edge is the result of individual experimentation by a collection of authors. Some of its features work very well (such as top-level factories for creating promise/future/etc.) and others not so much (such as the shift from JavaScript-inspired nomenclature to Scala). There are also edge features that simply aren't ready (like channels). So we need a plan and we need better coordination.

Over the next several days I will make several additional posts clarifying what I have been thinking about over the past several months. I'll post them as individual Issues. At a minimum I'll post 1) unifying philosophies and guidelines, 2) a consistent nomenclature and API, and 2) a rough roadmap. We can discuss each of these openly in their respective threads.

@pitr-ch
Copy link
Member Author

pitr-ch commented Jun 15, 2016

I agree with @jdantonio If I have comments I'll add them to the issues, Jerry will open.

Just to point out this PR had changed from a merge to core to an edge update, so there is no confusion around that. It kept the Namespace changes so it will not move again when moved to core. I feel strongly that this library (new promises not edge) should go to core soon and deprecate what it replaces. Deprecation will keep compatibility until 2.0 but new users will be advised to use new promises instead of the old ones. I would like to see this for 1.1 but that is probably a topic for one of the new issues.

Couple of reasons why:

  • Integrates older Future, Promise, Delay, ScheduledTask [1], Dataflow into one framework allowing seamless combining
  • Resulting API is (in my view) more clear and simpler
  • Its faster (in come cases many times faster). With exception of wait it never blocks thread or uses Mutex, instead it's build in volatile fields and CAS operations
  • The API is quite rich it allows everything I've ever seen elsewhere in similar libraries, it adds new ideas.
  • All methods taking blocks allow args passing ensuring safe publication (afaik old implementation lack it)
  • Cancellation is cooperative and independently provided and can be used in all other abstractions as well. (This one needs to be polished.)
  • Verified in production, it's used by Dynflow which backs big projects like (http://theforeman.org/, http://www.katello.org/)
  • With #run method can be used to simulate green-threads without requiring an actual Thread (less convenient then Fibre but does not create extra threads as Fibres on JRuby(+Truffle)) (This one needs to be polished.)

[1]: Not entirely true for ScheduledTask, needs more convenience api methods or just internal revision to integrate better with new promises.

There is one point of disagreement: the method naming JS vs Scala. I lean to the current naming and I would like to resolve this before merging this PR to master. The current naming resembled Scala, but the it is just because I found it the most clear naming, if there is a better one I'll change it. Shifting to JS names would be motivated by rather to create familiarity for our users. I would rather pick the most clear names independently on other libraries, because we'll be stack with them for long time. However I am not dismissing the value of making the API familiar, we could do aliases or a mapping in documentation for those familiar with JS promises (This is the option I prefer). This library has pending, completed, success, failed what I am missing in JS terms (pending, -, fulfilled, rejected) is counterpart for completed. completed with fulfilled, rejected does not work that well, it's not clear in my opinion.

@pitr-ch
Copy link
Member Author

pitr-ch commented Jun 24, 2016

There are two more argument why we should move quickly with this if possible. The gem has many downloads which is great achievement, but I suspect it's mostly as a Rails dependency not necessarily a direct usage of the gem. Although thanks to Rails new users are coming and starting to use the gem. We should get them on the new API so they can avoid migration. Even though right now our users are happy we know that the old API has few problems. We do not have to wait for our users to rediscover them and open issues, we can move forward providing a better tool.

@jdantonio
Copy link
Member

@pitr-ch Your arguments in favor of moving fast are not compelling.

I agree that the vast majority of the downloads are due to our big users: Rails, sprockets, and Sidekiq. Which means that the number of direct users is rather small, but very advanced group. They are very capable of handling a measured migration. The new users coming to the gem will also be advanced programmers capable of handling a measured migration. They've survived numerous upgrades to Rails over the years. The burden imposed by a migration to c-r 2.0 will be trivial by comparison.

Far worse than 1 migration, however, would be 2 migrations. If we move too fast with the migration to 2.0 and we get it wrong we'll then need a 3.0. It's far better to build a 2.0 that lasts a long time.

Part of this disagreement, I believe, is that we view the APIs in Edge differently. I think you believe that the APIs in Edge are basically "done" and ready for core. They aren't. The Edge APIs are experimental and need much discussion. There are things in those APIs that need changed, and those changes need open discussion. The reason we haven't discussed them at length yet is that you were busy working on the internal synchronization layer. I didn't feel the need to discuss the APIs while you were still doing that work.

I'd also like to remind you that your work on the synchronization layer is the reason we are in the predicament. My original plan was to release 1.0 a year ago so that 2.0 could ship with Rails. I moved that date back at your request so that you could do more work on the synchronization layer. I then moved the date back for you again. And again. And again. We didn't release 1.0 until November, fully four months later. Had we worked to my original schedule we would have shipped 2.0 with Rails. Instead, I agreed to work to your schedule and we missed that window. And now we are here.

For 2.0 we will work on my schedule. We will work at a deliberate and measured pace. We will review and openly discuss all of the API changes in Edge and decide which ones need updated before becoming part of core. We will develop a consistent set of API rules, guidelines, and terms. We will update Edge features to match the new rules/guidelines/terms before moving them into core so that Edge users can migrate first. And we will ship 2.0 when it is ready--just like we did with 1.0. Even if it takes longer than some members of the team would like.

@jdantonio
Copy link
Member

@pitr-ch Regarding: "There is one point of disagreement: the method naming JS vs Scala. I lean to the current naming and I would like to resolve this before merging this PR to master."

We will absolutely resolve this issue before merging to master. Honestly, I consider it resolved. The naming will be mostly inspired by JavaScript. I appreciate that you find the Scala naming to be most clear, but my primary concern is for the larger user community. JavaScript is used by far more programmers than Scala. JavaScript is used by far more Ruby programmers than Scala. JavaScript and Ruby programmers have reasonable expectations regarding promises based on their extensive use in JavaScript. We've already seen numerous cases where we've created confusion by being insensitive to these expectations. Our promises (and futures/tasks/etc.) will be designed correctly for Ruby and it's runtime, but we'll also be sensitive to the reasonable expectations of our users.

One of the documents I am currently working on is a set of proposed, consistent APIs. That document will include promise nomenclature based on JavaScript. We will discuss the specifics of the common APIs openly, but the inspiration for promises (JavaScript vs. Scala) is no longer open for debate.

@jdantonio
Copy link
Member

I've added the paused label to this PR. There are discussions that need to happen. I'm beginning to post separate issues to discuss the direction of 2.0. Work can certainly continue on this PR but it can't be merged until after we've had those other discussions. I'll remove the paused label once the entire team is aligned.

@jdantonio jdantonio modified the milestones: 2.0.0, 1.0.3 Jun 24, 2016
@jdantonio
Copy link
Member

To help alleviate confusion I'm reposting here a comment I made on another thread:

To clarify: we will not be throwing away Edge and starting over. Overall Edge is a fantastic evolution of this library. We absolutely will base 2.0 on the work done in Edge, with most (if not all) of it being retained. We just need to be universally aligned as a team, which we clearly aren't right now. Settling on nomenclature, for example, is a surface issue only and won't affect the core functionality. Similarly, consistency in how we handle internal construction (such as autoload vs. require) won't affect the core functionality. Moving Edge features to core in a deliberate and controlled manner (versus slamming everything in at once without proper discussion and alignment) won't effect the core functionality. But taking the time to do all these things right--rather than acting prematurely on impulse and raw desire--will be of vastly more benefit to the entire user base. If we are to be successful with the migration from 1.0 to the very different 2.0 we must put user experience first.

@pitr-ch
Copy link
Member Author

pitr-ch commented Jun 25, 2016

I agree that the vast majority of the downloads are due to our big users: Rails, sprockets, and Sidekiq. Which means that the number of direct users is rather small, but very advanced group. They are very capable of handling a measured migration. The new users coming to the gem will also be advanced programmers capable of handling a measured migration. They've survived numerous upgrades to Rails over the years. The burden imposed by a migration to c-r 2.0 will be trivial by comparison.

Agreed.

Far worse than 1 migration, however, would be 2 migrations. If we move too fast with the migration to 2.0 and we get it wrong we'll then need a 3.0. It's far better to build a 2.0 that lasts a long time.

Agreed, I certainly do not want to create another migration.

Part of this disagreement, I believe, is that we view the APIs in Edge differently. I think you believe that the APIs in Edge are basically "done" and ready for core. They aren't. The Edge APIs are experimental and need much discussion. There are things in those APIs that need changed, and those changes need open discussion. The reason we haven't discussed them at length yet is that you were busy working on the internal synchronisation layer. I didn't feel the need to discuss the APIs while you were still doing that work.

Yeah that was a misunderstanding, new futures are around for long time, you didn't provide much feedback therefore I mistakenly started to think you are ok with the API (except the state naming which was mentioned before somewhere). Now I understand more why.

My apologies, I've pushing this forward perhaps unnecessarily strongly, which created bad picture. I certainly do not want to be too fast leading to another migration or push it before there is space for discussion. My intention is given the guidelines (which we already agree on (except advanced features)) are met and discussion around API finished, then to move fast as possible. (The arguments I've given were in this scope.)

TL;DR I think this is the part of Edge which should go to core first, without breaking any guidelines.

Currently I am thinking following which is afaik conforming to our guidelines.

  1. Finish API discussions.
  2. Merge this PR, and major edge version bump (namespace is changing), advertising it as a new futures coming to core in next minor core release.
  3. give people time to try them
  4. moving to core (no API change), minor core bump, deprecating old futures

At the beginning this PR was admittedly going too fast, it was just step 1. and then going straight to 3. as a prerelease and to 4. I've since then realised that as the discussion continued.

I'd also like to remind you that your work on the synchronization layer is the reason we are in the predicament. My original plan was to release 1.0 a year ago so that 2.0 could ship with Rails. I moved that date back at your request so that you could do more work on the synchronization layer. I then moved the date back for you again. And again. And again. We didn't release 1.0 until November, fully four months later. Had we worked to my original schedule we would have shipped 2.0 with Rails. Instead, I agreed to work to your schedule and we missed that window. And now we are here.

Sorry for the delays, it's hard to estimate how much free time I'll have and how much time it will ultimately take to finish the contribution.

For 2.0 we will work on my schedule. We will work at a deliberate and measured pace. We will review and openly discuss all of the API changes in Edge and decide which ones need updated before becoming part of core. We will develop a consistent set of API rules, guidelines, and terms. We will update Edge features to match the new rules/guidelines/terms before moving them into core so that Edge users can migrate first. And we will ship 2.0 when it is ready--just like we did with 1.0. Even if it takes longer than some members of the team would like.

I think agreed except the schedule. What schedule do you have in mind tough? Are you thinking about having dates for future releases? That might be hard to do since it's hard to predict contributions to OSS. Releasing minor versions repeatedly and rather often when there is something new added, and releasing major when we want to cleanup deprecated might work better. I am not sure what your vision is.

One of the documents I am currently working on is a set of proposed, consistent APIs. That document will include promise nomenclature based on JavaScript. We will discuss the specifics of the common APIs openly, but the inspiration for promises (JavaScript vs. Scala) is no longer open for debate.

This is not public yet, is it?

You've mentioned there are API changes you would like to propose:

  1. Naming: you feel strongly about JS naming, I understand that you want to close, I certainly do not want to escalate, but even if I compromise on JS names there is still a name missing for :completed. JS promises do not have one, there is resolve constructor which could be used to infer resolved but other promise libraries has different names like settled. So it's not that easy. What do you think about just having fulfilled and rejected as aliases for success and failed? Another option would be to rename just success and failed to fulfilled and rejected keeping rest unchanged, I like it less but it would work too.
  2. Executor API: depends on 2.0 Some Specific Technical Changes #542
  3. Any others?

@jdantonio
Copy link
Member

I'm working on several documents to expand on all of these things, but the JavaScript naming is easiest to address. First, JavaScript promises DO have a formally documented state for completion. It's called fulfilled. Promise was one of the first things I created in this library. I followed the Promise/A and Promise/A+ specifications. This has always been in our documentation and our implementation has always had this state. Moving to the Scala naming didn't solve anything. It was a change that I've expressed dislike of many times. And it never solved any problems. The issue of JavaScript inspired names has been solved in this library from the earliest versions. All we need to do is stick with what we already have.

@pitr-ch
Copy link
Member Author

pitr-ch commented Jun 25, 2016

Ok so that means pending, completed, fulfilled and rejected. I'll update the PR. Sorry for the prolonged discussion on this.

@pitr-ch
Copy link
Member Author

pitr-ch commented Jul 30, 2016

States renamed to fulfilled etc. General constructor added. The documentation for new futures can be found temporarily here for review.

@pitr-ch pitr-ch force-pushed the futures branch 5 times, most recently from 4e9532d to e03cccc Compare December 24, 2016 15:41
@pitr-ch pitr-ch merged commit ad52e28 into ruby-concurrency:master Dec 27, 2016
dLobatog added a commit to dLobatog/dynflow that referenced this pull request Dec 27, 2016
The latest version, 0.3.0 moved futures to promises and the API
was renamed. See the changes on:
ruby-concurrency/concurrent-ruby#522
dLobatog added a commit to dLobatog/dynflow that referenced this pull request Dec 29, 2016
The latest version, 0.3.0 moved futures to promises and the API
was renamed. See the changes on:
ruby-concurrency/concurrent-ruby#522
iNecas pushed a commit to Dynflow/dynflow that referenced this pull request Dec 29, 2016
The latest version, 0.3.0 moved futures to promises and the API
was renamed. See the changes on:
ruby-concurrency/concurrent-ruby#522
devinmcgloin pushed a commit to devinmcgloin/concurrent-ruby that referenced this pull request Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding features, adding tests, improving documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants