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

Make change tracking and the update pipeline compatible with AOT/trimming #29761

Closed
roji opened this issue Dec 4, 2022 · 11 comments · Fixed by #33409
Closed

Make change tracking and the update pipeline compatible with AOT/trimming #29761

roji opened this issue Dec 4, 2022 · 11 comments · Fixed by #33409
Labels
area-aot area-change-tracking area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. consider-for-current-release type-enhancement

Comments

@roji
Copy link
Member

roji commented Dec 4, 2022

Note that although ExecuteUpdate/Delete allow bypassing change tracking and the update pipeline, we have no ExecuteInsert.

@kant2002
Copy link

Explanation of this thing a bit unclear. I read title as that some internal work for making code trimmable is needed. But in the description it sounds like introduction of ExecuteInsert which can bypass change tracking and update pipeline will do the trick. I'm just looking for easy NativeAOT related work in this repo, so curious is this task can be interpreted as such?

@roji
Copy link
Member Author

roji commented Dec 25, 2022

@kant2002 the note above was only meant to say we have the query-based update and delete APIs (ExecuteUpdate/Delete) which bypass change tracking and the update pipeline, so the ongoing work to make the query pipeline compatible with NativeAOT would also make it possible to update and delete, even if change tracking and the update pipeline aren't compatible. However, EF still does not allow insertion without going through change tracking and the update pipeline (#29897 tracks adding that), so making change tracking and the update pipeline trimming/AOT-compatible is necessary.

As always, you're welcome to look at doing NativeAOT comaptibility work for these components; but I'm afraid that wouldn't be "easy" work for anyone not familiar with EF internals. If you do want to dive in, it would be good to post a quick plan detailing what you intend to do, so we can agree on the approach before you spend too much time on implementation etc.
Unfortunately, I'm not sure there's "easy" NativeAOT work

@kant2002
Copy link

I understand that I cannot just land on codebase such EF Core and "just do NativeAOT", some amount of struggle is expected. But I still prefer finding place which is easier, since that allow me work on more or less self-sufficient manner.

I think most important question regarding NativeAOT/trimming work is how do you plan to test changes?

  • Is there already some test infrastructure for that, or something should be added?
  • Should this test infra to be created first before some work should be done?
  • Is this something outsider can help with? Maybe the plan is just to enable PublishTrimming/PublishAot somewhere and that's it.
  • is there set of specific scenarios/tests which should pass in NativeAOT/trimming worlds so this can be considered done?

This is generic questions, so maybe they should be in parent issue, don't know.

@roji
Copy link
Member Author

roji commented Dec 26, 2022

@kant2002 are you asking about how NativeAOT/trimming test infra works in general (outside of EF)? Or about the EF approach to testing NativeAOT/trimming? At the moment there's almost nothing in place (except for an extremely minimal trimming console app which provides only very basic coverage).

The exact scope of EF compatibility with NativeAOT/trimming (and therefore, what is covered by testing) will probably become apparent as the work progresses. In an ideal world, we'd be able to use the existing EF test suite, and simply run it with NativeAOT publishing; that would leverage the extremely comprehensive test coverage which EF already has, rather than starting from scratch. Unfortunately, there are likely to be various hurdles there, with xUnit NativeAOT compatibility possibly being one.

So I'm not quite sure how to answer your questions.. I'm hard at work at the moment on #25009 - precompiled queries - which are pretty much a prerequisite to NativeAOT support. Testing is something I intend to think about later in the process.

AndriySvyryd added a commit that referenced this issue Jul 13, 2023
AndriySvyryd added a commit that referenced this issue Jul 20, 2023
Use a variable for the TFM to make future upgrades easier.

Part of #29761
ghost pushed a commit that referenced this issue Jul 20, 2023
Use a variable for the TFM to make future upgrades easier.

Part of #29761
AndriySvyryd added a commit that referenced this issue Jul 22, 2023
Throw when invoking model building or migrations in a NativeAOT context

Part of #29761
AndriySvyryd added a commit that referenced this issue Jul 27, 2023
Throw when invoking model building or migrations in a NativeAOT context

Part of #29761
AndriySvyryd added a commit that referenced this issue Aug 7, 2023
All type mapping types now need to be public and have a public static property `Default` of its own type.
Annotate NTS as not supported with NativeAOT.

Part of #29761
AndriySvyryd added a commit that referenced this issue Aug 7, 2023
All type mapping types now need to be public and have a public static property Default of its own type.
Annotate NTS as not supported with NativeAOT.

Part of #29761
@dschulzDyconTech

This comment was marked as off-topic.

@roji

This comment was marked as off-topic.

@dschulzDyconTech

This comment was marked as off-topic.

@roji

This comment was marked as off-topic.

@AndriySvyryd AndriySvyryd modified the milestones: Backlog, 9.0.0 Jan 9, 2024
AndriySvyryd added a commit that referenced this issue Jan 26, 2024
* Generate change-tracking delegates in the compiled model.

Part of #29761
AndriySvyryd added a commit that referenced this issue Mar 26, 2024
Add full support for service properties
Use a consistent pattern for unsafe accessors
Store the unsafe accessor names in an annotation
Refactor the different mechanisms for tracking variables in the generated code into a single bidirectional dictionary

Fixes #29761
Fixes #24904
Fixes #24900
@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Mar 26, 2024
@AndriySvyryd AndriySvyryd removed their assignment Mar 26, 2024
@AndriySvyryd AndriySvyryd modified the milestones: 9.0.0, 9.0.0-preview4 Mar 26, 2024
AndriySvyryd added a commit that referenced this issue Mar 26, 2024
Add full support for service properties
Use a consistent pattern for unsafe accessors
Store the unsafe accessor names in an annotation
Refactor the different mechanisms for tracking variables in the generated code into a single bidirectional dictionary

Fixes #29761
Fixes #24904
Fixes #24900
AndriySvyryd added a commit that referenced this issue Apr 4, 2024
Add full support for service properties
Use a consistent pattern for unsafe accessors
Store the unsafe accessor names in an annotation
Refactor the different mechanisms for tracking variables in the generated code into a single bidirectional dictionary

Fixes #29761
Fixes #24904
Fixes #24900
AndriySvyryd added a commit that referenced this issue Apr 4, 2024
Add full support for service properties
Use a consistent pattern for unsafe accessors
Store the unsafe accessor names in an annotation
Refactor the different mechanisms for tracking variables in the generated code into a single bidirectional dictionary

Fixes #29761
Fixes #24904
Fixes #24900
@roji
Copy link
Member Author

roji commented Apr 4, 2024

@AndriySvyryd should we keep this open to track getting to zero warnings for change tracking/update pipelin?

@AndriySvyryd
Copy link
Member

At this point I think it would be more appropriate to have an overall issue to drive warnings to 0

@roji
Copy link
Member Author

roji commented Apr 4, 2024

OK, opened #33478. Note that there's already #29759 tracking getting to zero warnings for the 2nd part of the query pipeline, which presumably would be more on my plate - so maybe good to keep those two issues apart (though we can do whatever).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-aot area-change-tracking area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. consider-for-current-release type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants