Disable automatic query client evaluation #14935
Comments
Issue #14935 * Tests are disabled if their primary purpose is to test queries. * Queries are re-written if the purpose of the test was not primarily to test the query. * All are tagged with this issue number so they can easily be found.
Issue #14935 * Tests are disabled if their primary purpose is to test queries. * Queries are re-written if the purpose of the test was not primarily to test the query. * All are tagged with this issue number so they can easily be found.
Issue #14935 * Tests are disabled if their primary purpose is to test queries. * Queries are re-written if the purpose of the test was not primarily to test the query. * All are tagged with this issue number so they can easily be found.
Issue #14935 * Tests are disabled if their primary purpose is to test queries. * Queries are re-written if the purpose of the test was not primarily to test the query. * All are tagged with this issue number so they can easily be found.
ajcvickers
added
the
closed-fixed
|
Why was the decision made to not allow us to turn this off with the below? I agree that it should be on by default to prevent poor performance, but now this means I have to extensively test my application to make sure some moron didnt write bad code somewhere. This means we will never upgrade to EF Core 3.0. Too much time involved to test. |
|
Allowing to turn it off, means EF Core would perform automatic client eval. That is additional code to write and maintain. |
|
There is no way that it is the much code to write and maintain. No way i believe that! Do you not understand the impact, we have 100s of web api endpoints that return data. Unfortunately many are missing tests, so there is no way we can go and test them. I cant possibly be the first person who pushed back and said we need a way to turn this off. |
|
@darl0026 - You are the first one to ask a way to turn this off as evident by comments on this issue. We would consider a well-written PR with tests to add a way to client eval for all query operators in EF Core codebase if you would like to contribute. |
|
@jjxtra - Please stop spamming unrelated issues with your group by query is not working. |
|
@darl0026 Not to sound blunt but then, don't upgrade to 3.0 if you're not ready / don't have the time to do so. Its a major version upgrade so breaking changes are to be expected. We're still on 2.2 for this exact reason and I don't think we will be upgrading for a while yet. Also consider automatic integration testing / end to end testing, that way, if this happens again, you don't have to spend lot of man hours testing for breaking changes, you'll already have a compiled list of all the functionality that is broken and that you need to fix. To be honest, Id rather it happen now in 3.0 rather than in 9.0 and the code base is bigger, its just something you'll have to deal with, or use something that gives you more control over your queries i.e Dapper. I advocate for the usage of CQRS to split out the database queries to use Dapper where I get fine control on the query, and EF Core on the commands which gives me benefits such as the change tracker and unit of work pattern. |
|
As a dev manager with over 40 developers working for me, I have to agree with @darl0026 At the very least can you provide a tool that could search our codebase in VS.NET as an addin or something to help us find the breaking code up front and re-write. Or if you cant do that, how about providing some code sample to allow us to turn on logging in production so we log the call stack of failing methods so at least we can build up a picture of our code that needs changing up front? We could dump that into a table or azure table storage and then try to collate all the code that needs fixing up front. Otherwise what you are telling us to do is to set 2.2 to throw exceptions not warnings, run some code, let a method fail, fix it, compile it, run it, go to next method to test? That could literally take weeks/months in a large code base, and unless all code is covered by automated tests (not unit tests due to mocking of course, as that wont help here). I estimate the cost to my company to do that would be between £20,000-£100,000 which means an impossibility for us financially. So you guys REALLY have to do something to help us out. If we had known about the client side execution when we started on EF 2, of course we would have turned it on. But it is way too late for that now. |
|
@dougiejay assume you mean "turned it off" ? And how did you miss it in the first place? We're you coming from EF6? |
|
@ErikEJ "How did we miss it in the first place?" - As in how did we let something run in it's default mode? Not sure I follow. Surely you put the more dangerous setting (client eval) as the non-default setting that requires a customer to actively opt-in to the situation, not the other way around? And really, voting down a comment from a corporate client of Microsoft seems pretty petty and shows a disdain for users of the product that you promote and work on. |
|
@dougiejay EF Core versions prior to 3.0 emitted warnings to the log whenever client evaluation occurred; you could tell EF to throw instead - see the documentation section on client evaluation in previous versions. To configure logging, see this page. However, note that in EF Core 3.0 the query pipeline was also rewritten, so what involved client evaluation in 2.2 may not necessarily throw an exception in 3.0. The best way forward here would be to have a test/preproduction environment running on EF Core 3.1 and to use that to identify problematic queries. Hopefully you also have a test infrastructure to make this process easier. |
|
Again you guys are missing the point. Everyone is happy you have stopped client side evaluation. It was something that we would have avoided if we knew about this feature in 2.0 a year and a half ago (instead of being simply logged to VS. Net console). My point is that you have not provided any upgrade path or tools to assist migration. Yes we have over 600 automated api tests which will help with the migration process, however one api might have multiple db calls, and maybe several will fail due to the client eval, meaning to get one api call passing might take several builds taking significant time. When professional development is conducted by corporate clients like us, we expect a bit more than a serious breaking change and no assistance whatsoever from Microsoft to minimise cost of upgrading. If you don't understand my viewpoint then not much more to say and I will cease posting on this thread. |
Spot on agree with @darl0026 |
I do understand your point, and also the frustration with the situation. This is just my opinion/view, but had it been feasible/possible to provide some sort of automated tool to assist migration, we'd definitely have done it (for example, an analyzer to flag queries which would fail). However, doing this kind of analysis at compile time is an enormous investment (it really is), and time was already very tight to the 3.0 release (which there was no option to postpone). At the end of the day, we could have either made the change without a migration tool, or not have made the change at all - that's just the reality we had to face when making that decision.
FWIW the only downvote I'm seeing is from @ErikEJ who isn't a Microsoft employee (although he's an important member of the community). I don't think anyone in the EF team takes the client evaluation change lightly, or imagines that the transition would be trivial for customers; it's just that everyone agrees that dropping client evaluation was the right thing to do, and there was no feasible way to provide automated upgrade analysis etc. |
|
@darl0026 I'm very sorry to hear you are having trouble updating to EF Core 3.1. We certainly didn't take the decision lightly to overhaul the query code in the way we did for 3.0. However, after much discussion, we decided it was necessary for several reasons, some of them around robustness and user experience going forward, some of them internal relating to dependencies. With a change of this size it was clear regressions in existing functionality would occur, even outside the intended breaking changes. We have a small team (five engineers and a manager) so mitigations for this needed to be realistic. We focused on:
It was not realistic for us to create any kind of static analysis tools for this--we simply don't have the resources for that. It's also not clear to me how many cases could be reasonably picked up by static analysis alone. In addition, the problem space for LINQ queries is extremely large, which means that it is very easy for patterns to be missed even with tests we run. This means, realistically, using a new release of EF Core (whether it is a breaking release or not) requires that the application test with that new release to identify any issues. This is part of the reason for making frequent preview releases and daily builds available--we hope to encourage and foster relationships with customers so as to tighten the feedback on finding and fixing issues. This was an extreme case of a breaking release--we don't plan to make this kind of break routinely. However, we are committed to moving the product and experience forward in ways that may be breaking. We are certainly open if you have ideas for how we can further improve in these areas, within the constraints of the resources we have. |
|
Thanks for the heads up @ajcvickers, appreciate you being transparent. |
As a developer, I understand that it's sometimes necessary to make breaking changes, and the LINQ execution change is one I heartily endorse for its potential for improved performance. As someone who has to communicate with stakeholders, I shudder at the thought of trying to sell an EF Core 3.0 upgrade to them. "First we have to end-to-end test the entire application to determine what breaks in EF Core 3.0, then we need to spend an unknown amount of time fixing and testing the broken stuff." How many stakeholders are going to give the go-ahead for that, especially when the upside is "better performance... maybe"? IMO, the execution engine rewrite is a large enough change that it warranted a special release plan. Personally, I would have released EF Core 2.2.1 that had no change apart from upgrading the client-side execution warning to an error, which would have given developers adequate notice that their current code was problematic and should be changed. Then I would have released EF Core 3.0 with the new engine, and nobody would've had a reason to complain.
I believe that the big issue that @darl0026 and others, including myself, have is that the breaking nature of EF Core 3 vs older versions WRT query evaluation was not sufficiently communicated. The fact of the matter is that for any EF-using app of decent size, upgrading to EF Core 3 is probably going to cause breakage somewhere due to the new evaluation model, and that isn't called out particularly well. The announcement for EF Core 3.0 is peppered with weasel words like "can" and "may" and "could", when realistically any reasonably-sized app is going to be affected by these changes. What was, and arguably still is, needed on that blog post is a preface consisting of a big block of text on a red background saying something like: "Breaking changes: Because EF Core 3.0's internal expression compilation engine has been completely rewritten, existing queries may require modification to work with this version. We strongly recommend that you thoroughly test your application with EF Core 3.0 in order to identify affected queries, and use that as a guideline to determine if updating to this new version at this time is optimal for your software. Read on for details of the changes and potential modifications." Yes, that blog post did mention and link to the breaking changes, but its emphasis on the "breaking" part was not there, nor was it front and foremost as it should have been. I can understand why Microsoft and the EF team would be reluctant to potentially scare people away from their software, but the fact of the matter is that EF Core 3.0 is not just a drop-in upgrade, and any issues it potentially causes might only come to light days or months or years later. That's a massive business risk, and Microsoft needs to be responsible and disclose that up-front. Finally, the fact that there are so many questions on Stack Overflow regarding this exact topic (https://stackoverflow.com/q/59677609/70345 being only the most recent) is a strong indication that the existing language around migrating to EF Core 3.0 is not strong or clear enough. |
|
@IanKemp Thanks for the feedback; we appreciate your thoughts and as with all feedback we will consider it as we strive to continually improve. To address some of your specific points/suggestions:
In my opinion, this is something that needs to be done when updating any dependency that contains as much complexity as an OR/M. At one extreme, we try to be conservative with patch releases to reduce the chance of regressions. However, patch releases do sometimes still contain regressions, so its always important to test when updating a dependency, even for just a patch. This becomes even more important when taking a new minor version that has a lot of bug fixes and new features, but no planned breaking changes. And then more so for new major versions. EF Core 3.0 was at the extreme of this continuum, but it's still true that I have worked with customers who were able to update without anything breaking.
This would have been a major breaking change in a patch release, which isn't really on the table as an option. Also, the query code was fundamentally changed in 3.0. Just because something was being client evaluated in 2.2 doesn't mean it is client evaluated in 3.0. The reverse is also true to a lesser extent. So it would have given a false sense of security for the 3.0 update.
It was certainly not our intention to try to hide the breaking nature of this release. In fact, we have been striving for the opposite--to make sure as many people as possible are aware. This was front of mind for us in every preview, announcement, and blog post throughout the 3.0 cycle. Your feedback that this was not clear is certainly something we will consider when we make breaking changes in the future.
I would like to emphasize again that, in my opinion, no new release (other than possibly patches of simple code) should be considered a drop-in upgrade, regardless if that is an EF release or a release of something else containing considerable complexity. |
|
Got into issues with this breaking change while upgrading our project to 3.1 Was able to fix most of the simpler queries with AsEnumerable(), but we have few places where the query is built dynamically. For example the user can choose many search parameters and then we build the query with AsQueryable() and depending on choices Where() and GroupBy() are selectivelly added to the query before we finally call ToList() to fetch the data: In this scenario we can't insert AsQueryable() in the middle of the query build chain as it's not IQueryable anymore ... I was happy with EF that was able to determine the best way to manage those requests and return me results. At the moment I can't imagine how I can do this manually and will probably end up in even worse performances because of the complexity and number of tables my dynamic query needs to handle ... Si I guess i'll have to stick with 2.2 until I find some solution or hopefuly client evaluations might come back as optional query paramater in some future EF version ... |
|
@peter-bozovic note that there aren't any plans to bring back client evaluation at the moment. While the upgrade from 2.x to 3.x is indeed painful because of this, we believe that EF Core shouldn't be transparently pulling back an entire table because something couldn't be translated in a Where clause. If you want to dynamically add search parameters which would get evaluated on the client side, you can still do that by composing an IEnumerable expression tree and compiling that. But once again, you're the one who knows whether it makes sense for your users that you perform arbitrary queries in a potentially extremely-inefficient way (which is what 2.2 did for you). |
|
Agreed, simply use |
|
We were also stuck in upgrading to EF Core 3 due to this issue. Is there any chance to get the QueryClientEvaluationWarnings out of a production system running 2.2 with a hint of where the problem occurs (e.g call stack) We could then run 2 weeks in production, fix all problematic code and then upgrade to 3.x with minimized chance of breaking. Maybe something supporting this kind of tracing can be implemented in a 2.2 patch if it's not possible out of the box. |
You must be kidding. This has got to be the worst example of premature optimization I've ever encountered in my professional career. You have broken thousands of advanced queries written by people that depended upon IQueryable's graceful degradation to IEnumerable up until 3.x. You can no longer use ternary operators on select projections, string concatenation, or complex select projections without intermediate stupidity like forcing evaluation & execution in the middle of the code. Hell, right now you can't even use functions like Contains or pass a predicate lambda via a method call to a where clause, so a client that expects to work on an IEnumerable (even if EF has already cached the result for Linq to Object manipulation) breaks when it gets an IQueryable expression that can no longer gracefully degrade to IEnumerable. Another example: Did you create a code first model with unmapped, calculated values to make queries simpler and less mistake prone, knowing that such things would be evaluated on the client after the full set (that matched a server side predicate) was pulled down? No more, now you must either explicitly force an evaluation via AsEnumerable() (or otherwise separate those two calls), or push the calculation into the server where clause. Elegant, code first calculated properties are now less valuable in 3.x than they were in 2.2. Why? to prevent a "mistake" that the developer fully intended? This was a major advantage of EF, now gone. So, let me summarize the upshot here: This sucks. It was EF's flexibility and forgiving nature of accepting any valid LINQ expression for both data retrieval & client side data manipulation as it seamlessly toggled between LINQ to EF and LINQ to Objects that made it the best ORM available on ANY platform. EF's capabilities were the sort of thing you'd dangle in front of Java developers who didn't even know what they were missing over beers. Now we get to return to the world where 3NF querying concerns must be thought of as separate and distinct from OOP client manipulations. "We would consider a well-written PR with tests to add a way to client eval for all query operators in EF Core codebase if you would like to contribute." This is nearly the same tone and attitude expressed when the dotnet core System.Data team decided to remove the interfaces for IDbConnection/IDBTransaction, in their initial pass because the EF Core team thought abstract classes would be good enough. (thereby violating a core principle of SOLID). Of course MS eventually reversed themselves as the comments by reputable ORM developers shredding that decision piled up, but not before making all sorts of similar unhelpful statements. I know you're volunteers, but EF Core is an important project and platform for developers (especially commercial developers) everywhere, so let me put it this way, hopefully without being similarly dismissive: Nah. I'll just be sure to drop EF from the stack next time and go back to Dapper. Dapper's reliable. I thought EF was too, but evidently that is not the case anymore. |
|
Core philosophy of IQueryable is not to evaluate anything rather build an expression tree to allow the query provider to process it whatever way they want. So we haven't broken core philosophy of IQueryable in any form. You may want to reconsider your understanding of IQueryable vs IEnumerable. |
EOL for 2.2 was 1 month after your comment. |
|
|
@MarkMcTernan I 100% agree with you. see my comments from Dec 2019. we STILL havent had capacity to move to 3.1x, over 8 months later. That is the reality of the breaking change in the real world for a large enterprise organisation. |
|
@MarkMcTernan where have you had Azure removing 2.2? that terrifies me as we are still in production on 2.2. |
|
If you look on the documentation for Azure's AppService, you'll see that 2.2 is not among the advertised frameworks. On June 19th, AzureDevOps removed 2.2 from their docker build agents on Mac, Linux, and Windows. This was announced here on June 1st: For me, the DevOps build agents broke first (I run xunit as part of the CD pipeline), but after fixing that I discovered it also applied to the App Service docker images themselves, because a successful deploy started throwing 500 errors for the same 'missing 2.2 framework' issue. The MS support rep confirmed for me that build agents and app services apparently rely upon the same cached Docker images. That was only a few days ago. I assumed it would be ok so long as I didn't run a deployment to my prod environment while I worked on the upgrade. That lasted a few days before my prod* App Service, which I never deployed to, started throwing 500 errors for the same reason, so apparently Azure attempted to spool up a running instance on a new container even though I had the "always on" feature set to true. *A caveat: My customer's "prod" is a dev/test B2 instance (B2 supports SSL, always on, and other 'prod' features). Maybe they won't touch the prod tier App Services, but I wouldn't bet on that after my experience. |
|
I understand the frustration with the breaking changes here. I would have very much preferred a configuration property on the context options to opt into this new behavior, with the default being 2.2 behavior. I hope this is an option for .net 5. I also understand the maintenance burden of having two code paths or implementations so I don’t take this kind of request lightly. |
This is planned for 3.0 anyway--see #12795. However, by disabling now for the next preview, we can start getting feedback on the break to prepare for when the 3.0 query changes are in a state to be shipped.
The text was updated successfully, but these errors were encountered: