New 2.7/3.0 keyword argument pain point

Hi,

I’m going to speak from the point of view of a developer who is in charge of moving our rails monolith to ruby 2.7.

What I missed in the first place was a real strategy. Over 50 000 warnings generated by our CI, thousands from gems. I didn’t know where to start. So I waited, to see if a strategy emerges, or if the gem maintainers can quickly fix their warnings.

Then thanks to the twitter thread of Eileen Uchitelle and the contribution of Rafael França and Kevin Deisz (here, and here), I found something that could work in our case:

  1. Fix the errors related to this change Double splat with an empty hash ( **{} ) passes no arguments – about ten tests out of 15,000. Instead of a ActiveRecord::Relation we had a ActiveRecord::QueryMethods::WhereChain.

  2. Ignore deprecation warnings: warning.rb · GitHub. Thanks to this, I avoid flooding our log in production and in development.

  3. Use DeprecationToolkit from Shopify – Thanks ! – to list existing warnings in our codebase and avoid adding new ones. Thanks to this configuration, I ask him to take into account the warnings coming only from our codebase and not from the gems: deprecation_toolkit.rb · GitHub

  4. Fix them all.

With this approach, we will be able to merge ruby 2.7 quickly, then in a second step correct the warnings from our codebase in the coming weeks. This makes the transition much less painful on our side.

In conclusion, from my opinion:

  • New keywords argument are better than the old ones
  • What I needed was not more time, but a clear strategy to make the transition right

I hope that helps. :slight_smile:

20 Likes

@matz, I think putting this in for Ruby 3 is a great idea, and I’ll be sad to see this go. Is there a reason why Ruby 3 needs to be shipped this Christmas? Maybe there could be a Ruby 2.8 this year where some more of these kinks get worked out, and then the feature can land in Ruby 3 next year.

10 Likes

@Florent_Beaurain thanks for mentioning deprecation_toolkit, I should have also talked about how we re dealing with warnings.

We didn’t keep a close tab on how much time we actually did spend on fixing warnings, but a wet finger estimate would be 3 devs on and of over the course of two weeks, so maybe something like 1 week for 1 dev full time. That’s for a repo on which hundreds of people work every day, so in the end it’s a really small fraction of our usual development effort.

What I’m trying to say is that seeing so much warnings when you upgrade is really overwhelming, but in the end it’s not that much effort to upgrade as long as you silence the ones coming from your dependencies. It’s really nothing compared to the work we had to put in most Rails upgrade over the years.

So I wholeheartedly agree with your conclusion. Ruby is only missing two things:

  • An easy way to forward parameters that works on all currently supported versions.
  • Some communication around the tools available as well as some kind of migration guide.
6 Likes

I agree with you 100%. I didn’t over-calculate. But I can come up with relatively similar numbers on a 500k LOC codebase and 80+ devs.

Very well summed up. That’s exactly what I think. I’m going to write a blog article on how we’re handling this thing at Doctolib. Hope this will help.

6 Likes

I was personally prepared to deal with the pain for the benefit, I thought it was worth it, and was looking forward to rationalized kw args. But avoiding backwards incompat pain is super important, so I respect I may have been wrong with regard to the community as a whole!

This matches my experience – this is the biggest problem I see or have had with current not entirely rational kw args.

There are other use cases for this general thing, which I’d describe as: Capturing arguments “arbitrarily” to forward them on later. Without being sure exactly what args the method in question defines. Keyword arguments? New-style, or old style as a hash? Other optional arguments? Etc.

Another use case is overriding a method, to look at/modify certain known args, while capturing and forwarding “all other” args, while being “future compatible” with it’s changing exactly what methods it has or how it defines them (switching from last arg options = {} to **options etc).

def some_method(*position_args, **kw_args)
   position_args[1] ||= something_else
   kw_args[:other] = another_thing
   other_method_with_same_args_unchanged(*position_args, **kwargs)
end

Right now, it is very hard (impossible?) to do that in a reliable way, that won’t change how the ultimate receiving method receives them. The example above, I think, won’t reliably do it while working consistently in recent ruby versions.

If there were a way to do this (even using special-purpose syntax or methods?), I think the “irrationality” of keyword arguments otherwise would seldom get in the way.

Hi. Not rails-related, but I’ve stumbled across this thread, and since I’m in the middle of quite a bit of pain related to these changes, I thought I might want to share.

I thought I was being a good Ruby citizen, and slowly migrating my code to using keyword arguments, double splat and all that. I (fortunately for me) am not keeping very long backward compatibility across ruby versions, so I can do this type of things.

I basically thought I was all set. A few days of fixing the (very welcome) warnings and that’s it. Boy was I wrong.

I ended up hitting the decision of allowing arbitrary keyword arguments in 2.7, at the same time than the other changes - my (rightfully rejected) bug report is here.

To be honest, I felt very cheated. I thought I was doing the right thing with a slow upgrade, and keyword arguments have rejected anything that’s not a Symbol since day 1. So, I (stupidly) assumed that it would stay that way at least until the “real” keyword args arrive (a.k.a. after 3.0). Now, were something like that happen again, I’d basically go the route of wait and change stuff only when strictly required.

It turns out that the “user-facing” API of my library (a robotic framework) is a DSL, and some methods are given both a mapping of strings to strings and keyword arguments. But then, I thought I’ll be fine (because there’s no way they can break that, strings can’t go in kw args). However, what was working before:

def provides(h = {}, **kw); end
provides "some" => "strings", as: "name"

now resolves everything into the keyword splat. I’m scared of even changing things now, since adding a keyword splat will break code. If I would change the following signature:

def m(mapping = {}, as: nil); end

into one that has a keyword splat - for instance to forward arbitrary arguments to another method -

def m(mapping = {}, as: nil, **kw); end

then what was going into mapping now goes into kw

This change IMO makes no sense from a backward compatibility perspective. It is a change that could definitely have waited for 3.1 after the real keyword arguments are here and here to stay, where everyone properly separated hashes from keywords. I’ve been through the 1.9 transition, and I feel like that.

Don’t get me wrong, I like the idea of using 3.0 as a way to finally fix the kw args mess, and 2.7 as a stepping stone to there. And I am biting the bullet and trying to get this to work.

But this is not that. This is a backward-compatibility-breaking change that was there not to clean up in the way to 3.0, but to add new functionality that was not there before.

As for the warnings, we would ideally need a way to disable them per-package. Users of rails or other libraries/frameworks shouldn’t have to care that the framework needs updating.

Finally, last but not least, Ubuntu 20.04 shipped with Ruby 2.7. This is going to be a lot of pain for Ruby developers on Ubuntu, and I believe will fuel the dislike of Ruby for that platform’s users.

1 Like

At Discourse we still have not upgraded to Ruby 2.7 in production. I think it is quite a shame, and the biggest driver for not upgrading was that the benefits of Ruby 2.7 were small compared to dealing with various loud deprecations. We did not notice that Discourse runs any faster on 2.7 maybe it is all the deprecation accounting, I don’t know.

Recently, we upgraded to latest Rails to reduce warnings, but we are left with warnings from gems and warnings from Discourse core.

Examples of Discourse warnings are:

/home/sam/Source/discourse/lib/freedom_patches/inflector_backport.rb:30: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/home/sam/.gem/ruby/2.7.1/gems/activesupport-6.0.3/lib/active_support/inflector/methods.rb:128: warning: The called method `humanize_without_cache' is defined here
/home/sam/Source/discourse/lib/plugin/instance.rb:230: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/home/sam/.gem/ruby/2.7.1/gems/activemodel-6.0.3/lib/active_model/callbacks.rb:130: warning: The called method is defined here
/home/sam/Source/discourse/lib/plugin/instance.rb:230: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/home/sam/.gem/ruby/2.7.1/gems/activemodel-6.0.3/lib/active_model/callbacks.rb:144: warning: The called method is defined here
/home/sam/Source/discourse/app/models/user.rb:212: warning: deprecated Object#=~ is called on Array; it always returns nil
/home/sam/Source/discourse/app/models/user.rb:212: warning: deprecated Object#=~ is called on Array; it always returns nil

Examples of gem warnings are:

/home/sam/.gem/ruby/2.7.1/gems/nokogumbo-2.0.2/lib/nokogumbo/html5.rb:22: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/home/sam/.gem/ruby/2.7.1/gems/nokogumbo-2.0.2/lib/nokogumbo/html5/document.rb:4: warning: The called method `parse' is defined here
/home/sam/.gem/ruby/2.7.1/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil
/home/sam/.gem/ruby/2.7.1/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil
..../home/sam/.gem/ruby/2.7.1/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil
../home/sam/.gem/ruby/2.7.1/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil
........./home/sam/.gem/ruby/2.7.1/gems/sassc-2.0.1/lib/sassc/script/value/string.rb:26: warning: deprecated Object#=~ is called on TrueClass; it always returns nil

Dealing with our warnings is feasible albeit annoying. More on this soon. Dealing with gem warnings can be a nightmare.

For example to deal with the sassc we are going to have to start with upgrading sassc we saw recent versions of this gem segfault so we have held back for a bit, getting us to latest is probably us spending weeks of debugging. Further more, gem authors take time to approve PRs and push new versions out there (understandably) so this is going to take a while.

Just silencing deprecations feels wrong to us as a path forward. If Ruby, by default, is being loud I feel we just have to deal with it.

Dealing with the Discourse side of things now comes with some fundamental problems. Firstly I am uneasy with the whole ruby2_keywords thing, it feels like I am adding technical debt to my code when I hack it in. This is all code that I am going to have to remove / re-implement.

Half of our “we did it to ourselves” warnings are coming from our Inflector memoizer. https://github.com/discourse/discourse/blob/4601833e4ed98aaaaf90c0ddab2bfebfae42db7c/lib/freedom_patches/inflector_backport.rb#L8-L40. The Rails inflector proved to be a bit of a hot spot for us (and Shopify also confirm that some methods are for them) so we introduced an LRU cache in front to memoize methods.

In Ruby 2.6, a naive implementation is very easy to reason about, it goes something like this:

def memoize_26(method_name)
  cache = {}

  uncached = "#{method_name}_without_cache"
  alias_method uncached, method_name

  define_method(method_name) do |*arguments|
    found = true
    data = cache.fetch(arguments) { found = false }
    unless found
      cache[arguments] = data = public_send(uncached, *arguments)
    end
    data
  end
end

It can be done a bit faster, but the code is easy to reason about and it has worked for years.

Now Ruby 2.7 comes along and I need to call ruby2_keywords on this dynamic method (and check if we can do this) and carry tech debt going forward, or implement it differently.

... is not going to work for me, cause I need the values passed in so I can do my cache lookup.

Simply adding **kwargs unconditionally to this memoizer means that now my memoizer just got slower for methods with no keyword args, I need to construct a new array for the cache lookup with both kwargs and args

So if I want something that is usually as fast as it used to be I am stuck with this:

def memoize_27_v2(method_name)
  uncached = "#{method_name}_without_cache"
  alias_method uncached, method_name

  cache = "MEMOIZE_#{method_name}"

  params = instance_method(method_name).parameters
  has_kwargs = params.any? {|t, name| "#{t}".start_with? "key"}
  has_args = params.any? {|t, name| !"#{t}".start_with? "key"}

  args = []

  args << "args" if has_args
  args << "kwargs" if has_kwargs

  args_text = args.map do |n|
    n == "args" ? "*args" : "**kwargs"
  end.join(",")

  class_eval <<~RUBY
    #{cache} = {}
    def #{method_name}(#{args_text})
      found = true
      all_args = #{args.length === 2 ? "[args, kwargs]" : args[0]}
      data = #{cache}.fetch(all_args) { found = false }
      unless found
        #{cache}[all_args] = data = public_send(:#{uncached} #{args.empty? ? "" : ", #{args_text}"})
      end

      data
    end
  RUBY

end

Even with all this wizardry I can not recover the performance of the old *args capture cause I am stuck constructing a new array for the cache lookup if args and kwargs are there at the same time.

My old method which was easier to reason about is 1.3 times faster cause I am not stuck generating an array for the lookup.

Full benchmark here:

What do I think should be done

Huge +1 on

  1. Allow for some new syntax that allows a global capture of both kwargs and args (...a is not going to work cause it will clash):
def foo(bar)
end

def x(***a)
   
   p a

   foo ***a
end

# maybe
def x(...)
   a = ...
   p a
   foo ...
end 
  1. Remove all kwarg deprecation accounting and warnings from Ruby 2.7

  2. The train has probably left the station but ruby2_keywords… I wish it could be removed, if we had support for ***a in 3 we could just eval ourselves out of the syntax clash in a switch for the places where we need cross Ruby support. I don’t know.

  3. Consider, why did we allow so many deprecations to go silent for years and suddenly we are very loud?

irb(main):001:0> URI.escape("a")
=> "a"
irb(main):002:0> RUBY_VERSION
=> "2.6.5"

# vs

irb(main):001:0> URI.escape("a")
(irb):1: warning: URI.escape is obsolete
=> "a"
irb(main):002:0> RUBY_VERSION
=> "2.7.1"

This for example has been deprecated for almost 15 years already, but just now got loud.

I extremely appreciate everyone in the MRI core team and I understand the enromity of this conundrum, but sadly the loud deprecation notices are holding people up from upgrading Ruby which is achieving the opposite of one of the core missions of the MRI team which is to keep everyone upgraded.

Discourse / Basecamp / Shopify / GitHub not running 2.7 yet at scale means that tons of real world performance testing is not happening and all sorts of bugs will take a lot longer to find.

@eileencodes has been working on upgrading GitHub recently, perhaps she has more insight.

I know @rafaelfranca has also been very vocal about the delegation change, perhaps he has some stuff to add here.

6 Likes

Actually we’re running it since end of March. Sorry if my message was confusing.

I might just be misunderstanding you, but I don’t think anyone is advocating about just silencing deprecations. What we’re advising people to do it to first upgrade to 2.7 with warnings silenced. And then use the warning and / or deprecation_toolkit gem to fix them in smaller chunks rather than in one big commit.

There is no shame in silencing deprecation as long as you have the process in place to grind them down over time.

In my mind there is a big problem that need to be addressed for gem authors, because it’s way too hard to write code that works without deprecations on both 2.6 and 2.7.

But for end user applications, IMHO the issue is blown out of proportion. Some Rails upgrades took us over a year of developer time (e.g. protected attributes removal), cleaning our codebase to be compatible with the real keyword args took a week, maybe two max.

4 Likes

The key change I am struggling to get my head around is the direction change the core ruby team took.

For the last 8 versions of ruby if you ran it with no special flags stuff was mostly silent. If you wanted to clean house you would turn on warnings. 2.7 introduced a series of loud deprecation notices, so to me the urgency of dealing with everything just “feels” higher.

Many JS frameworks warm loudly on almost every new release regarding upcoming deprecations, and I have grown accustomed to it.

It is just this transitional period for ruby where the ground rules have changed that I am finding tricky to deal with.

2 Likes

@samsaffron that’s fair. I think we can agree that such a verbose deprecation warning being immediately introduced at the default $VERBOSE level is unprecedented and might feel rushed. It probably all come down to wanting to go from 2.7 to 3.0 and wanting to have real keyword arguments in 3.0.

I’d also agree that there’s lessons to be learned from this.

But now that it’s a thing, might as well keep a cool head and think rationally about it. Yes it is a very noisy warning, but no it’s not that big a deal for application maintainers (definitely is for library maintainers).

3 Likes

I think deprecation warnings have almost always been shown by default in Ruby, otherwise people might miss it, and then it would just break when, e.g., the method is removed. That said, I think keyword arguments warnings are far more numerous than any deprecation warning before.

I opened this ticked on the ruby bug tracker to discuss the performance issue I mentioned in my post:

Completely agree we got to keep a cool head here. We are certainly going to get through all of this this. I love how caring Matz and team are about these niggles.

For context Discourse core which is a huge code base only has 2 places that need

if Module.respond_to?(:ruby2_keywords, true)
  ruby2_keywords(m)
end

Other deprecations we have already addressed in our custom rubocop rules here: rubocop-discourse/lib/rubocop/cop/discourse at main · discourse/rubocop-discourse · GitHub

Specifically: rubocop-discourse/no_uri_escape_encode.rb at main · discourse/rubocop-discourse · GitHub which was silent in 2.6 but became noisy in 2.7.

The bigger challenge we have is around upstreaming fixes to gems, but we will certainly also get through that.

2 Likes

For Ruby3, the real keyword argument is not the primary benefit (but JIT, concurrency, and static type analysis are). It’s kind of a side dish along with type analysis because the traditional keyword argument is harder to analyze.

Matz.

11 Likes

I totally agree with you.

I think it will be really nice if we can do correct things on ruby 3, if Ruby 2 keyword arguments is a mistake(i think matz said on different occasions), i think update to ruby 3 is the best time to fix it.

I can guess because rails is use too many ruby magic, so, update is painful, but, rails can always select to use old ruby version 2.7. (or a more less warn version, e.g. 2.7.2) and moving to ruby 3 slowly, i even if ruby core team have enough resources, we can release ruby 2.8 with ruby 3 same time, but, i really propose to let ruby evolution toward the correct way.

Thank you.

1 Like

Awesome! Very glad to hear that these are the priorities.

Note, for those interested in this topic it is now being discussed as well in the official Ruby bug tracker:

2 Likes

I think my main concern, personally, is a bit of a meta one.

It’s important to be able to evolve an API forwards over time, and deprecations are an excellent tool for doing so.

The point of a deprecation is that old-style code continues working for a transition period, while developers are encouraged to adopt the new form, without breaking any existing behaviour.

Any time a deprecation is shown to the developer who is responsible for the line that generated the deprecation warning, that is a good thing, and allows them to address the change, in a neat and orderly manner, before the later hard-break change arrives.

(I think all the above is pretty uncontroversial. So now we move on to:)

However, any time a deprecation is shown to someone who is not responsible for the code that caused the warning, that is a breaking change.

If my ruby-authored CLI tool one day starts spitting out warnings to my users, the tool is broken.

If my library/framework starts spitting out warnings (which are the library authors’ responsibility to fix before some future ruby release), that library is broken. And worse, it’s obscuring any warnings that might belong to code written by the local developer.

By going immediately noisy with deprecation warnings, they in practice become an immediate breaking change, and for a lot of people, you might as well have just made the real change without warning and started raising exceptions.

After that, recommending that everyone install warning-filtering code to suppress the warnings just feels tone deaf. If the warnings are immediately important, then why suggest people suppress them? If they’re not, why force everyone to update their code just to get back the previous [warning-free] functionality?

This warning seems to have been added, and defaulted-on, on the assumption that the original authors are the only people who ever see their code’s stderr. (Or that they’re sufficiently involved in ruby-core to recognise the future warning before it shipped, and adapt their code in advance – thereby illustrating that they too do not consider it acceptable for a library to cause un-actionable warnings for its consumers.)

I have mixed feelings about the actual change in question (in short: edge cases can be annoying, but don’t find myself running into the problem this is solving; in my personal opinion, this is forcing a widespread change to code that’s worked consistently since at least 1.8, and it doesn’t feel worth it). But that’s very secondary to how it’s been made. Many libraries have already adapted, small and large – including Rails. But that was forced, on a fast timeline, because downstream users (reasonably, and sometimes loudly) considered those libraries to be broken on Ruby 2.7.

“Is broken on the new Ruby release” is not a deprecation, and if users are perceiving it as broken, then it is broken.

As “just leave Rails behind on old Ruby, and let Ruby evolve on its own” has been suggested, I’ll briefly address it: that’s exactly our concern! There are limits to how far into the future things can be expected to be compatible, but if, when we release Rails, we can’t even expect Ruby+1 to run our code correctly (and here “correctly” means our library doesn’t cause any automatic output/warnings/errors we couldn’t anticipate using the previous version, and isn’t attributable to calling code), then I think the only way we can give our users a smooth experience is to apply a max ruby version as well as a minimum.

For the record, this is something we struggle with when deprecating Rails APIs too. When we deprecate things used by libraries more than application code, we generally try to run a cycle of soft (silent) deprecation first… but we absolutely do get it wrong, and force hurried changes upon our downstreams. Maybe we need our own equivalent of -v. In truth, I think we mostly get away with it because people expect the new version of Rails to break their library, either outright, or with noisy deprecations… and expect upgrading to be a planned, effort-heavy, operation. Historically, I believe people have assumed that ruby-core is more cautious, and so a minor version bump will be a safe and low-pain change. If the plan is to reverse that expectation, then you do get more freedom to change things faster… but you also get a lot more people left behind on old versions.

8 Likes

This is extremely well-said. The tension between Ruby for library-makers and tool-makers versus Ruby for end-users of those tools is very clear, and the rules around how version changes “break” things need to consider both.

Walter

To followup on my previous post, I started clearing deprecations in our gems yesterday, the first step being to list the gems throwing deprecations.

Out of the 332 gems in our Gemfile, only 37 are printing warnings, only 23 of them are public gems, and if I also remove the gems that are public but used almost exclusively by shopify I’m down to a very small list of 17 gems:

rails # actually it's Rails code calling ours (`as_json`). The fix is to be done inside our code.
activemerchant
bugsnag
faraday
google-cloud-storage
grpc
i18n
memcached_store
minitest # same, it's our code called by minitest that expects keyword args.
mocha # same, our code that expects keywords when it is actually a hash.
rails-controller-testing
responders
sorbet-runtime
state_machines-activemodel
state_machines-activerecord
statsd-instrument
webmock

And for some of them like faraday, the work was already done upstream, we just have to upgrade.

Of course this is only one datapoint, even if our Gemfile is big it’s not representative of the community as a whole, but I think it’s fair to say that the gem ecosystem either wasn’t as much impacted as one might think, or either already started to support Ruby 2.7.

5 Likes

I agree with you. However, I wonder if you have any idea how to address the issue you raise. How do you change something and get libraries to update without any kind of visible warning in user code?