-
Notifications
You must be signed in to change notification settings - Fork 512
V2.0 refactor #1235
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
base: main
Are you sure you want to change the base?
V2.0 refactor #1235
Conversation
No tests fixed yet.
phsyicsnemo.utils, launch.config is just gone. It was empty.
|
/blossom-ci |
CharlelieLrt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems some imports haven't been fixed. For instance I still see in the examples some imports like:
from physicsnemo.models import ModuleThose are not expected to work anymore I think.
* Ensure that the pip dependency group exclusively matches the resolution of the dependency-groups from Pep 735 that uv enables. * Revert some dependency tracking. Now, we allow unbare external imports anywhere. But the import list is smaller, and more easily managed with uv / pip equally. extras are now chained and built in the optional deps. * Add dev deps. Add docstring for ci script. * Update pyproject with small changes from review.
|
/blossom-ci |
- Update links to physicsnemo-curator examples in DoMINO and Transolver READMEs. - Update link to CachedDoMINODataset and path reference for cache_data.py script.
|
/blossom-ci |
Alexey-Kamenev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked some of the models, LGTM.
| include = ["physicsnemo", "physicsnemo.*"] | ||
| ##################################################################### | ||
| # Linting configuration | ||
| ##################################################################### | ||
|
|
||
| [tool.ruff] | ||
| # Enable flake8/pycodestyle (`E`), Pyflakes (`F`), flake8-bandit (`S`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Below on the line 173 it says "Never enforce E402," but E402 is not in the ignore list.
| @@ -901,6 +899,9 @@ def __init__( | |||
| ) | |||
|
|
|||
| def getFNOEncoder(self): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such non-standard naming will be addressed later I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Alexey-Kamenev can you elaborate on what you mean by "non-standard" naming? I can see two things wrong there:
- This method should be private
- This method name should be using snake case
So, essentially _get_fno_encoder instead of getFNOEncoder.
Were you thinking of anything else?
@ktangsali for viz, since he is assigned to this task
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CharlelieLrt - that's right, _get_fno_encoder is exactly what I meant!
| @@ -221,7 +200,7 @@ def forward( | |||
| self, | |||
| node_features: Tensor, | |||
| edge_features: Tensor, | |||
| graph: Union[DGLGraph, List[DGLGraph], CuGraphCSC], | |||
| graph: Union[GraphType, List[GraphType]], | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I forgot to change it to a more appropriate type.
|
/blossom-ci |
|
/blossom-ci |
| [tool.uv] | ||
| no-build-isolation-package = ["torch_scatter"] | ||
| managed = false | ||
|
|
||
|
|
||
| [project.entry-points."physicsnemo.models"] | ||
| AFNO = "physicsnemo.models.afno:AFNO" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coreyjadams I realized something wrong with the automatic registration of Modules. Now the registration happens at (sub)class definition. This might lead to unexpected behaviors.
For instance:
from physicsnemo.core import ModelRegistry
model_registry = ModelRegistry()
model_registry.list_models()
# ['AFNO', 'DLWP', 'EDMPrecondSR', 'FNO', 'Fengwu', 'FullyConnected', 'GraphCastNet', 'MeshGraphNet', 'One2ManyRNN', 'Pangu', 'Pix2Pix', 'SRResNet', 'SwinRNN', 'UNet']If after that I do:
from physicsnemo.models.diffusion import SongUNet # Defines multiple Module subclasses: SongUNet, SongUNetPosEmdb, etc...
model_registry.list_models()
# ['AFNO', 'DLWP', 'EDMPrecondSR', 'FNO', 'Fengwu', 'FullyConnected', 'GraphCastNet', 'MeshGraphNet', 'One2ManyRNN', 'Pangu', 'Pix2Pix', 'SRResNet', 'SwinRNN', 'UNet', 'SongUNet', 'SongUNetPosEmbd', 'SongUNetPosLtEmbd', 'DhariwalUNet', 'StormCastUNet', 'VPPrecond', 'VEPrecond', 'iDDPMPrecond', 'EDMPrecond', 'EDMPrecondSuperResolution']This type of implicit stateful is very counter-intuitive. This could be changed by having all the models from the zoom put here in the entry points. Not sure it's the best option though.
peterdsharpe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed most of our open questions in the refactor meetings + over Slack, this looks good to me! I've been building on top of this for a few weeks now and haven't experienced any issues. The addition of uv-compatibility for pyproject.toml is particularly nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coreyjadams those are layers, shouldn't they go into physicsnemo.nn instead?
Or maybe they are not general purpose enough to put them in physicsnemo.nn? In that case maybe a better place is in physicsnemo/models/dlwp_healpix instead of the upper level physicsnemo.models (since these layers are really not models that are part of the zoo)
@pzharrington maybe you know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually yes, but I'm gonna request we postpone that migration until the follow-up work of addressing models which do not conform to standards.
CharlelieLrt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumped into a wrong import: from physicsnemo.models.meta import ModelMetaData
* Adding cpu ci * Make sure to get dev deps * target tests properly, maybe fix uv build * make sure radius search passes on CPU * maybe fix uv sync. Hopefully fix ubuntu testing. * Fix a test typo * Matrix to more python versions * drop support for python < 3.11 * fix link, enable nightly, target only selected branches * Only run python 3.12 on PRs, ubuntu and mac os, uv and pip * Add coverage to devs * Limit to one test path for PRs. nightly CI is consistent to all tests. uv install gets a retry.
…amples (#1276) * Remove TensorFlow dependency in Vortex Shedding and Lagrangian MGN examples * Update READMEs
PhysicsNeMo Pull Request
Rearchitecture of core physicsnemo components. Highlights:
modelsandmetricshave a few "-extra" dependencies that are unprotected still, to be fixed before merge.launch. Logging utilities moved intophysicsnemo.utils.logging.uv.Overall, this should lower the installation challenges and lay the groundwork for the rest of v2.0.
Description
Checklist
Dependencies
Review Process
All PRs are reviewed by the PhysicsNeMo team before merging.
Depending on which files are changed, GitHub may automatically assign a maintainer for review.
We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.
AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.