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

Fix/trace mode #1216

Closed
wants to merge 12 commits into from
Closed

Fix/trace mode #1216

wants to merge 12 commits into from

Conversation

sebffischer
Copy link
Collaborator

@sebffischer sebffischer commented Dec 6, 2024

Bugfixes:

  • save-load roundtrip used to convert script_function into script_module. This is fixed now (in a somewhat hacky way)

Open questions.

  • In this PR, we only do the train-eval distinction for the $forward method. Maybe we want to do this for all methods.
  • Can we register the $forward method (that distinguishes between the internal train and eval forward method) in the underlying script module instead of the nn_Module?
  • Reading a script function is currently loaded as a module, I don't think this should be the case

TODOs:

  • the training mode after saving a script_module is changed. This should probably be fixed.
  • Currently, after saving a trace-jitted module, the $forward method is not defined. This is because the underlying module class is not saved when saving a trace jitted module, so when loading it again, the unpatched methods are used.
  • Properly distinguish between saved module and saved function. The current hack does not work for jitted modules that were saved with an old version.

@sebffischer sebffischer marked this pull request as draft December 7, 2024 08:08
@sebffischer sebffischer marked this pull request as ready for review December 7, 2024 09:15
@sebffischer sebffischer marked this pull request as draft December 7, 2024 09:15
@sebffischer sebffischer marked this pull request as ready for review December 7, 2024 09:30
@sebffischer sebffischer marked this pull request as draft December 7, 2024 18:50
@sebffischer
Copy link
Collaborator Author

Superseded by: #1217

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant