Skip to content

Conversation

@garrettwrong
Copy link
Collaborator

This top level import (which I put in a long time ago for potential convenience) seems to be causing increased memory usage.

I don't think anyone uses the feature. Its also most of what makes the first import of aspire slow.

Tentatively suggesting we remove it. Running some large experiments now to see if improves the resource utilization.

@garrettwrong garrettwrong added bug Something isn't working Optimization Performance or Resource Optimzation labels Dec 11, 2023
@garrettwrong garrettwrong self-assigned this Dec 11, 2023
@codecov
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (347c779) 89.08% compared to head (6707f72) 88.76%.

Files Patch % Lines
src/aspire/__init__.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1064      +/-   ##
===========================================
- Coverage    89.08%   88.76%   -0.33%     
===========================================
  Files          126      126              
  Lines        11965    11969       +4     
===========================================
- Hits         10659    10624      -35     
- Misses        1306     1345      +39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@garrettwrong
Copy link
Collaborator Author

garrettwrong commented Dec 14, 2023

Updating with the dynamic importing compromise suggested by Joakim(numpy).

The getattr wrapper is now dynamically performing the importlib I was doing before at the top level.

This keeps the ability to do the following:

import aspire
s = aspire.source.Simulation()

@garrettwrong garrettwrong requested a review from j-c-c December 14, 2023 19:56
@garrettwrong
Copy link
Collaborator Author

@j-c-c Can you do a prelim review on this? I'm still running the tests regarding the resource usage, but I think we want this either way at this point. (I know its already improving some resource usage cases and has the best compromise regarding imports).

j-c-c
j-c-c previously approved these changes Dec 14, 2023
Copy link
Collaborator

@j-c-c j-c-c left a comment

Choose a reason for hiding this comment

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

This looks good to me. Everything seems to work as expected.

The only thing I would change (which is not functionally important) is adding backticks around {__name__} and {attr} in the error message.

@garrettwrong
Copy link
Collaborator Author

This looks good to me. Everything seems to work as expected.

The only thing I would change (which is not functionally important) is adding backticks around {__name__} and {attr} in the error message.

Sure, done.

@garrettwrong garrettwrong requested a review from j-c-c December 15, 2023 12:26
@garrettwrong garrettwrong marked this pull request as ready for review December 15, 2023 13:58
@garrettwrong garrettwrong requested a review from janden as a code owner December 15, 2023 13:58
@garrettwrong
Copy link
Collaborator Author

Merging.

@garrettwrong garrettwrong merged commit 8f94a24 into develop Dec 15, 2023
@garrettwrong garrettwrong deleted the importlib_unhack branch December 15, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Optimization Performance or Resource Optimzation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants