Fix #668: Safely handle empty custom_atlases.conf file #681

kalpana-pardeep-rassani · 2025-11-28T14:08:48Z

Before submitting a pull request (PR), please read the contributing guide.

Please fill out as much of this template as you can, but if you have any problems or questions, just leave a comment and we will help out :)

Description

What is this PR

Bug fix
Addition of a new feature
Other

Why is this PR needed?
The current implementation crashes with a KeyError: 'atlases' if the local custom_atlases.conf file exists but is empty (contains no sections). This prevents the brainglobe list command from running.

What does this PR do?
This PR modifies the get_all_atlases_lastversions function in brainglobe_atlasapi/list_atlases.py to use a safe .get("atlases", {}) method when reading the custom atlas configuration. This ensures that an empty dictionary is used if the 'atlases' section is missing, thus preventing the crash.

References

Fixes #668

How has this PR been tested?

The fix was tested locally by ensuring that the brainglobe list command runs successfully without crashing when the ~/.brainglobe/custom_atlases.conf file is present but completely empty.

Is this a breaking change?

No. This is a fix for an existing bug and does not change any public API or required functionality.

Does this PR require an update to the documentation?

No.

Checklist:

[done] The code has been tested locally
[done] Tests have been added to cover all new functionality (unit & integration)
The documentation has been updated to reflect any changes
[done] The code has been formatted with pre-commit
Note on Test Failures:

All code changes related to Issue #668 (empty config file) have been verified locally. When running the full test suite, I encountered 2 ConnectionErrors (due to the GIN server being unreachable) and 1 FileNotFoundError (due to missing local test data), which are external to this fix. The patch itself is clean.

alessandrofelder

Thanks @kalpana-pardeep-rassani

I unfortunately haven't had time for an in-depth review (hopefully by the end of this week) - apologies. I have some high-level comments for you though which are hopefully helpful. Let me know what you think.

alessandrofelder · 2025-12-01T14:27:52Z

brainglobe_atlasapi/list_atlases.py

+    # --- FIX APPLIED HERE ---
+    # Safely get the custom atlases data. If the 'atlases' section does not
+    # exist (e.g., file is empty), custom_atlases.get("atlases", {}) returns {}
+    # instead of crashing.
+    custom_atlas_data = custom_atlases.get("atlases", {})
+
+    return {**official_atlases["atlases"], **custom_atlas_data}
+
+
+# The get_atlases_lastversions function below this should remain unchanged.


Suggested change

# --- FIX APPLIED HERE ---

# Safely get the custom atlases data. If the 'atlases' section does not

# exist (e.g., file is empty), custom_atlases.get("atlases", {}) returns {}

# instead of crashing.

custom_atlas_data = custom_atlases.get("atlases", {})

return {**official_atlases["atlases"], **custom_atlas_data}

# The get_atlases_lastversions function below this should remain unchanged.

# Safely get the custom atlases data. If the 'atlases' section does not

# exist (e.g., file is empty), custom_atlases.get("atlases", {}) returns {}

# instead of crashing.

custom_atlas_data = custom_atlases.get("atlases", {})

return {**official_atlases["atlases"], **custom_atlas_data}

I think some of these comments relate to the PR, and not the code itself - so I don't think they belong in the code. Do you agree (I may have missed something)?

alessandrofelder · 2025-12-01T14:29:01Z

brainglobe_atlasapi/list_atlases.py

    return table
+
+
+def test_empty_config_file_no_crash(tmp_path):


This test looks reasonable at first glance but I think it should go somewhere in the tests folder with the other tests. It will be picked up by pytest there!

alessandrofelder · 2025-12-01T14:29:39Z

brainglobe_atlasapi/list_atlases.py

 """

 import re
+import warnings  # Adding this line


Suggested change

import warnings # Adding this line

import warnings

I don't think this comment is useful - it is very generic and not related to the line of code AFAICT

kalpana-pardeep-rassani · 2025-12-01T18:43:36Z

Thank you very much for taking the time to review this, especially with your busy schedule! I really appreciate the high-level comments. I have just applied an update to the branch to address this: Removed all explanatory comments Integrated the safe-access logic (.get("atlases", {})) cleanly inside the correct function body to maintain the original code structure. Request for Further Clarification If you have a moment, could you please provide a tiny bit more detail on whether this new structure resolves the issue? Also, if you see any problems with the warning placement or the logic in the unit test that I added, please let me know. I am committed to ensuring this fix is merged correctly and ready for your final approval! Thank you again for the guidance.

…

On Mon, Dec 1, 2025, 7:31 PM Alessandro Felder ***@***.***> wrote: ***@***.**** requested changes on this pull request. Thanks @kalpana-pardeep-rassani <https://github.com/kalpana-pardeep-rassani> I unfortunately haven't had time for an in-depth review (hopefully by the end of this week) - apologies. I have some high-level comments for you though which are hopefully helpful. Let me know what you think. ------------------------------ In brainglobe_atlasapi/list_atlases.py <#681 (comment)> : > + # --- FIX APPLIED HERE --- + # Safely get the custom atlases data. If the 'atlases' section does not + # exist (e.g., file is empty), custom_atlases.get("atlases", {}) returns {} + # instead of crashing. + custom_atlas_data = custom_atlases.get("atlases", {}) + + return {**official_atlases["atlases"], **custom_atlas_data} + + +# The get_atlases_lastversions function below this should remain unchanged. ⬇️ Suggested change - # --- FIX APPLIED HERE --- - # Safely get the custom atlases data. If the 'atlases' section does not - # exist (e.g., file is empty), custom_atlases.get("atlases", {}) returns {} - # instead of crashing. - custom_atlas_data = custom_atlases.get("atlases", {}) - - return {**official_atlases["atlases"], **custom_atlas_data} - - -# The get_atlases_lastversions function below this should remain unchanged. + # Safely get the custom atlases data. If the 'atlases' section does not + # exist (e.g., file is empty), custom_atlases.get("atlases", {}) returns {} + # instead of crashing. + custom_atlas_data = custom_atlases.get("atlases", {}) + + return {**official_atlases["atlases"], **custom_atlas_data} I think some of these comments relate to the PR, and not the code itself - so I don't think they belong in the code. Do you agree (I may have missed something)? ------------------------------ In brainglobe_atlasapi/list_atlases.py <#681 (comment)> : > @@ -228,3 +241,32 @@ def add_atlas_to_row(atlas, info, table, show_local_path=False): table.add_row(*row) return table + + +def test_empty_config_file_no_crash(tmp_path): This test looks reasonable at first glance but I think it should go somewhere in the tests folder with the other tests. It will be picked up by pytest there! ------------------------------ In brainglobe_atlasapi/list_atlases.py <#681 (comment)> : > @@ -4,6 +4,7 @@ """ import re +import warnings # Adding this line ⬇️ Suggested change -import warnings # Adding this line +import warnings I don't think this comment is useful - it is very generic and not related to the line of code AFAICT — Reply to this email directly, view it on GitHub <#681 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BGKRCRMWIMQLRVL4QW4IJ6L37RGMTAVCNFSM6AAAAACNPIEYL2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTKMRVGI3TKMZRGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

alessandrofelder · 2025-12-04T15:04:25Z

I don't think you've addressed any of my comments with your latest changes @kalpana-pardeep-rassani
Maybe something went wrong during pushing?

Most importantly, AFAICT, the test you wrote is in the wrong place, and will therefore not be run by pytest!

… all test and pre-commit style issues

kalpana-pardeep-rassani · 2025-12-08T08:58:08Z

those 'windows' and 'mac' (pull requests) are cancelled during test, how do i tackle this issue, please guide.. and also these skipped pull requesets what to do with them?

adamltyson · 2025-12-08T09:02:04Z

I've re-run the tests.

alessandrofelder

Thanks @kalpana-pardeep-rassani - there is more work needed on the tests. They could be simpler. I hope my comments are helpful.

alessandrofelder · 2025-12-08T10:53:17Z

brainglobe_atlasapi/list_atlases.py

        return dict(official_atlases["atlases"])
-    return {**official_atlases["atlases"], **custom_atlases["atlases"]}
+
+    # --- FIX APPLIED HERE ---


Suggested change

# --- FIX APPLIED HERE ---

This will not mean anything after this issue is fixed - it's too generic and makes the code harder to interpret. So I think it should be remove.

alessandrofelder · 2025-12-08T10:57:56Z

tests/atlasapi/test_list_atlases.py


-def test_get_downloaded_atlases():
+@pytest.fixture
+def mock_atlas_dir(tmp_path, mocker):


Most of the functionality in this fixture is already implemented in https://github.com/brainglobe/brainglobe-atlasapi/blob/main/tests/conftest.py and ready for you to use here, so I would suggest

removing it here

refactoring your test to use the existing functionality
so we don't have to look after duplicate code.

alessandrofelder · 2025-12-08T11:03:44Z

tests/atlasapi/test_list_atlases.py

    assert expected_print in captured.out
+
+
+def test_empty_custom_config_no_crash(tmp_path: Path, mocker):


I think it is sufficient to only add this test, and not change any of the other tests - they test things that are unrelated to the issue we are trying to fix.

I also think this test could be a lot simpler. You only need to check that

when you have an empty custom_atlases.conf file...
*... then the function doesn't crash, and possibly returns some existing atlases that are not custom.

It shouldn't need to be this long, or involve so many mocks. You can re-use some existing fixtures from conftest.py as I suggested in a previous comment.

adamltyson · 2026-01-27T11:46:59Z

Hi @kalpana-pardeep-rassani are you able to get back to this PR?

kalpana-pardeep-rassani force-pushed the fix/issue-668-empty-conf branch from b2d73aa to 49b8741 Compare November 29, 2025 10:06

alessandrofelder requested changes Dec 1, 2025

View reviewed changes

kalpana-pardeep-rassani force-pushed the fix/issue-668-empty-conf branch 2 times, most recently from cc46f34 to 6d53dd6 Compare December 1, 2025 18:16

kalpana-pardeep-rassani force-pushed the fix/issue-668-empty-conf branch from 6d53dd6 to 2749295 Compare December 4, 2025 14:56

kalpanapardeep added 2 commits December 5, 2025 17:46

temp commit for pre-commit

aa9dc0b

Fix brainglobe#668: Safely handle empty custom_atlases.conf; resolved…

b4e3c86

… all test and pre-commit style issues

kalpana-pardeep-rassani force-pushed the fix/issue-668-empty-conf branch from 708a630 to b4e3c86 Compare December 5, 2025 13:14

alessandrofelder requested changes Dec 8, 2025

View reviewed changes

		assert expected_print in captured.out


		def test_empty_custom_config_no_crash(tmp_path: Path, mocker):

Fix #668: Safely handle empty custom_atlases.conf file #681

Are you sure you want to change the base?

Fix #668: Safely handle empty custom_atlases.conf file #681

Uh oh!

Conversation

kalpana-pardeep-rassani commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

References

How has this PR been tested?

Is this a breaking change?

Does this PR require an update to the documentation?

Checklist:

Uh oh!

alessandrofelder left a comment

Choose a reason for hiding this comment

Uh oh!

alessandrofelder Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

alessandrofelder Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

alessandrofelder Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

kalpana-pardeep-rassani commented Dec 1, 2025 via email

Uh oh!

alessandrofelder commented Dec 4, 2025

Uh oh!

kalpana-pardeep-rassani commented Dec 8, 2025

Uh oh!

adamltyson commented Dec 8, 2025

Uh oh!

alessandrofelder left a comment

Choose a reason for hiding this comment

Uh oh!

alessandrofelder Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

alessandrofelder Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

alessandrofelder Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

adamltyson commented Jan 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kalpana-pardeep-rassani commented Nov 28, 2025 •

edited

Loading