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

REF: Update PtychoNN integration to 0.3.x API #87

Merged
merged 12 commits into from
Jun 27, 2024
Merged

Conversation

carterbox
Copy link
Contributor

@carterbox carterbox commented Jun 6, 2024

Migrates the PtychoNN integration to the 0.3.x API. This new PtychoNN API is functional instead of object oriented and uses Pytorch Lighting under the hood.

TODOS

  • Reduce precision of "Validation Set Fraction Size" slider to 0.1. We don't need to be able to choose precision to 1/1000th of a dataset
  • Remove "Optimization Epoch Per Half Cycle" field. It's not longer user settable.
  • Redirect "Status Interval" field to Trainer(log_every_n_steps=50). May need to adjust PtychoNN API.
  • Change UI section under "Save Training Artifacts". Path is a folder, and Suffix is the name of the model checkpoint/parameters inside that folder. It's unclear that Suffix must end in .pth in order for the model state to be loaded again by Ptychodus
  • Expose method for changing data parallelism model to single process

Comment on lines 109 to 120
if self._modelSettings.stateFilePath.value.exists():
path = self._modelSettings.stateFilePath.value
parameters = None
else:
path = None
parameters = dict(
nconv=self._modelSettings.numberOfConvolutionKernels.value,
use_batch_norm=self._modelSettings.useBatchNormalization.value,
enable_amplitude=self._enableAmplitude,
max_lr=float(self._trainingSettings.maximumLearningRate.value),
min_lr=float(self._trainingSettings.minimumLearningRate.value),
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior of createModel is now that a model is created if the stateFilePath does not exist, otherwise it tries to load the model from the existing file. Not sure if that was the previous behavior. This means that a model is always returned, but if the user provided an incorrect model path, they will get a randomly initialized model without any error.

@carterbox carterbox requested a review from stevehenke June 6, 2024 20:44
@carterbox
Copy link
Contributor Author

@stevehenke, this PR needs some help with the TODO list. I believe that the training works, but there are some GUI elements that are now awkward/obsolete, and I don't know where the implementation for those UI elements are located.

@carterbox
Copy link
Contributor Author

It looks like dp is no longer a distributed training option for pytorch lighting. The options are listed here.

Perhaps ddp_spawn might be the closest to what you are looking for?

@stevehenke stevehenke marked this pull request as ready for review June 27, 2024 18:46
@stevehenke stevehenke merged commit 4fd8eee into master Jun 27, 2024
1 check failed
@stevehenke stevehenke deleted the update-ptychonn branch June 27, 2024 18:48
@carterbox
Copy link
Contributor Author

Importing this this version fails if PtychoNN is not installed because you have imported pytorch lightning directly in order to use type hints. I believe this is called an API leak? Ptychodus is supposed to be agnostic to the PtychoNN backend, but since PtychoNN references lighting classes in its public API, Ptychodus needs to know about these classes. Probably I should reexport lightning.Trainer as ptychonn.Trainer such that a direct import of lighting is not needed by Ptychodus.

Comment on lines +4 to +5
import lightning
import ptychonn
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of these imports cause ptychonn to become a dependency for the base ptychodus package. I'm not sure how to avoid these imports, but ptychonn should not be a requirement to run ptychodus.

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.

2 participants