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

CellBuild topology page easily crashes when deleting sections with continuous create on #3005

Merged
merged 16 commits into from
Sep 16, 2024

Conversation

nrnhines
Copy link
Member

@nrnhines nrnhines commented Jul 18, 2024

At the last NEURON course, some users experienced issues with deleting sections in cell builder with continuous create on.
With this PR, the topology page now seems robust for all operations with continuous create.

There are likely some accumulating memory leaks during CellBuild usage and left over subsidiary objects when a CellBuild is closed. Those should be cleaned up in a separate pr.

The test_secref.py covers 97% of secref.cpp

When this PR is merged, it should be cherry picked into release/8.2

Copy link

codecov bot commented Jul 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.35%. Comparing base (e67effd) to head (53b87f7).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3005      +/-   ##
==========================================
+ Coverage   67.26%   67.35%   +0.09%     
==========================================
  Files         573      573              
  Lines      104927   104936       +9     
==========================================
+ Hits        70576    70684     +108     
+ Misses      34351    34252      -99     

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

Copy link

✔️ eb5a542 -> Azure artifacts URL

@nrnhines
Copy link
Member Author

The CI documentation error is

TypeError: canonicalize_version() got an unexpected keyword argument 'strip_trailing_zero'

I presume this will go away after a near future merge from master.

There is CI ubuntu error

ubuntu-22.04 - cmake (-DNRN_ENABLE_CORENEURON=ON -DNRN_ENABLE_INTERVIEWS=OFF -DNMODL_SANITIZERS=undefinedundefined)

92 - rxdmod_tests::rxd_tests (Failed)
test/rxd/test_parameter_reaction.py .                                    [ 83%]
4681
test/rxd/test_pltvar.py F                                                [ 84%]

I'm not familiar with the diagnostic process in this area. My presumption is that it is not related to this PR.

Copy link

✔️ 6321177 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@nrnhines
Copy link
Member Author

@pramodk
ci/gitlab/bbpgitlab.epfl.ch — Pipeline failed on GitLab
However, all the status checkmarks in bbpbuildbot above are green. Is everything in fact OK? If so, please approve this PR.

@nrnhines
Copy link
Member Author

nrnhines commented Aug 12, 2024

I just noticed a comment by @1uc at
#3037 (comment)
Is the gitlab failure an example of that temporary Sonata issue?
edit: Ah yes. I just saw the NEURONdev slack general message.

@1uc
Copy link
Collaborator

1uc commented Aug 12, 2024

Yes, this is the same issue. The pipeline that fails is called blueconfigs and it's not covered by the checkmarks. That's likely because blueconfigs is shared CI between several projects: NEURON, NMODL and neurodamus. Therefore, I think it was never taught to "report back" correctly for each of the three projects.

Rerunning should make CI go green again.

Copy link

✔️ 0b8fc13 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ d6ac77e -> Azure artifacts URL

Copy link

✔️ 34c8339 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ a8f7566 -> Azure artifacts URL

@pramodk
Copy link
Member

pramodk commented Sep 6, 2024

I have retriggered blueconfig CI. Hopefully, that will be green. As this is reviewed/discussed, feel free to merge once gitlab will done.

Copy link

✔️ d7d465c -> Azure artifacts URL

@pramodk pramodk added this to the Release v9.0 milestone Sep 9, 2024
Copy link

✔️ 53b87f7 -> Azure artifacts URL

@nrnhines
Copy link
Member Author

@pramodk Can you review? It's ready to merge as far as I can see.

@nrnhines nrnhines enabled auto-merge (squash) September 16, 2024 14:05
src/nrnoc/secref.cpp Show resolved Hide resolved
test/hoctests/tests/test_secref.py Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Sep 16, 2024

Copy link

✔️ 436c87c -> Azure artifacts URL

@nrnhines nrnhines merged commit 91090b4 into master Sep 16, 2024
35 of 36 checks passed
@nrnhines nrnhines deleted the hines/cellbuild-fix branch September 16, 2024 20:55
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.

5 participants