fix: propagate errors when datamanager fails to resolve paths for a refg #172

SophiaPerzan-DG · 2025-06-23T20:10:27Z

Fixes #171

If the datamanager failed when trying to resolve the path, the stack trace would get swallowed and the code would attempt to load the non-existent .cfg in order to parse it. This would then fail since the file doesn't exist, and print the misleading error message.

Created a new Error class for cases when data manager tries to find a file and the file does not exist. C++ backend uses this error type to determine which error message to print (and stack trace in the case of data manager failure)

Old behaviour:

Invalid genome

gk.Genome("hg38999")
Traceback (most recent call last):
File "", line 1, in
File "/Users/sophiaperzan/Desktop/dev/GenomeKit/genome_kit/genome.py", line 117, in new
genome = _cxx.Genome.new(cls, config)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
OSError: src/refg.cpp:56: Assembly is missing its chrom.sizes or annotation is missing its cfg file: hg38999
src/file.cpp:147: Could not open hg38999.cfg for reading (No such file or directory).

Valid genome, but error occurs within data manager when trying to get the file

This example shows what might happen if you have no GCS credentials for the GCSDataManager

gk.Genome("hg38")
Traceback (most recent call last):
File "", line 1, in
File "/Users/sophiaperzan/Desktop/dev/GenomeKit/genome_kit/genome.py", line 117, in new
genome = _cxx.Genome.new(cls, config)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
OSError: src/refg.cpp:56: Assembly is missing its chrom.sizes or annotation is missing its cfg file: hg38
src/file.cpp:147: Could not open hg38.cfg for reading (No such file or directory).

New behaviour:

Invalid genome

gk.Genome("hg3999")
Traceback (most recent call last):
File "", line 1, in
File "/Users/sophiaperzan/Desktop/dev/GenomeKit/genome_kit/genome.py", line 113, in new
genome = _cxx.Genome.new(cls, config)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
RuntimeError: src/refg.cpp:45: Assembly is missing its chrom.sizes or annotation is missing its cfg file: hg3999

Valid genome, but error occurs within data manager when trying to get the file

This example shows what might happen if you have no GCS credentials for the GCSDataManager

gk.Genome("hg38")
Traceback (most recent call last):
File "", line 1, in
File "/Users/sophiaperzan/Desktop/dev/GenomeKit/genome_kit/genome.py", line 113, in new
genome = _cxx.Genome.new(cls, config)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: src/py_init.cpp:78: Traceback (most recent call last):
File "/Users/sophiaperzan/Desktop/dev/GenomeKit/genome_kit/gk_data.py", line 101, in resolve_datafile_path
path = get_file(rel_path)
^^^^^^^^^^^^^^^^^^
File "/Users/sophiaperzan/Desktop/dev/GenomeKit/genome_kit/gk_data.py", line 67, in get_file
return data_manager.get_file(filename)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/sophiaperzan/Desktop/dev/GenomeKit/genome_kit/data_manager.py", line 212, in get_file
blob = self.bucket.blob(filename)
^^^^^^^^^^^
File "/Users/sophiaperzan/Desktop/dev/GenomeKit/genome_kit/data_manager.py", line 195, in bucket
gcloud_client = storage.Client()
^^^^^^^^^^^^^^^^
File "/Users/sophiaperzan/conda/envs/genomekit_test/lib/python3.12/site-packages/google/cloud/storage/client.py", line 247, in init
super(Client, self).init(
File "/Users/sophiaperzan/conda/envs/genomekit_test/lib/python3.12/site-packages/google/cloud/client/init.py", line 338, in init
_ClientProjectMixin.init(self, project=project, credentials=credentials)
File "/Users/sophiaperzan/conda/envs/genomekit_test/lib/python3.12/site-packages/google/cloud/client/init.py", line 286, in init
project = self._determine_default(project)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/sophiaperzan/conda/envs/genomekit_test/lib/python3.12/site-packages/google/cloud/client/init.py", line 305, in _determine_default
return _determine_default_project(project)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/sophiaperzan/conda/envs/genomekit_test/lib/python3.12/site-packages/google/cloud/_helpers/init.py", line 152, in _determine_default_project
_, project = google.auth.default()
^^^^^^^^^^^^^^^^^^^^^
File "/Users/sophiaperzan/conda/envs/genomekit_test/lib/python3.12/site-packages/google/auth/_default.py", line 685, in default
raise exceptions.DefaultCredentialsError(_CLOUD_SDK_MISSING_CREDENTIALS)
google.auth.exceptions.DefaultCredentialsError: Your default credentials were not found. To set up Application Default Credentials, see https://cloud.google.com/docs/authentication/external/set-up-adc for more information.

Copilot

Pull Request Overview

The PR updates error handling in refg_registry_t::as_refg to ensure resolution failures in the datamanager surface instead of being silently swallowed.

Replace empty catch with GK_RETHROW to propagate path-resolution errors.
Enhance the thrown error message with context about the failed annotation lookup.

src/refg.cpp

SophiaPerzan-DG · 2025-06-24T16:42:13Z

Closing temporarily after discussion with @ovesh. Will close the issue via a different approach.

… a file

ovesh · 2025-07-08T23:09:14Z

genome_kit/data_manager.py

+            "Please upload it using `genome_kit.gk_data.upload_file`."
+        )
+        self.filename = filename
+        super().__init__(self.message)


Why is this necessary?

It's not necessary, but it alters the exception message printing slightly.
With call to super().__init__

genome_kit.data_manager.GKDataFileNotFoundError: hg3814123213.cfg not found!

Without

genome_kit.data_manager.GKDataFileNotFoundError: ('hg3814123213.cfg', 'hg3814123213.cfg not found!')

I'd say with is slightly more in-line with python exception printing, but it doesn't ultimately matter IMO. I can remove if you'd like.

I guess the question is why don't we just throw the IOError?

ex:

GenomeKit/src/file.cpp

Line 100 in c073879

GK_CHECK(result == 0, file, "Error seeking to position {} in file ({}).", offset, strerror(errno));

if we do want to be more fine-grained, it'd be better to update the mapping and use the existing mechanisms:

GenomeKit/src/py_util.h

Lines 98 to 108 in c073879

GKPY_CATCH_CASE(gk::assertion_error, PyExc_AssertionError, return_stmt) \

GKPY_CATCH_CASE(gk::file_error, PyExc_OSError, return_stmt) \

GKPY_CATCH_CASE(gk::type_error, PyExc_TypeError, return_stmt) \

GKPY_CATCH_CASE(gk::value_error, PyExc_ValueError, return_stmt) \

GKPY_CATCH_CASE(gk::index_error, PyExc_IndexError, return_stmt) \

GKPY_CATCH_CASE(gk::key_error, PyExc_KeyError, return_stmt) \

GKPY_CATCH_CASE(gk::memory_error, PyExc_MemoryError, return_stmt) \

GKPY_CATCH_CASE(gk::not_implemented_error, PyExc_NotImplementedError, return_stmt) \

GKPY_CATCH_CASE(gk::unreachable_code_error, PyExc_RuntimeError, return_stmt) \

GKPY_CATCH_CASE(gk::runtime_error, PyExc_RuntimeError, return_stmt) \

GKPY_CATCH_CASE(std::exception, PyExc_RuntimeError, return_stmt) \

Actually I'm having trouble understanding what's wrong with just raising FileNotFoundError and doing a rethrow for whatever we get

Was this response meant for this thread? I'm not seeing the connection to calling super().__init__ for the exception. If you meant it in the sense of 'why are we defining a new exception for the data manager to throw, rather than just have it throw FileNotFound?', then I suppose that could be done. I chose to define a new exception type to differentiate it from FileNotFound in that GKDataFileNotFound signifies the data manager determined the requested file does not exist. The thinking here is if the data manager raises FileNotFound due to a different error somewhere, the c++ extension module can then tell an error occurred (and not just that a file doesn't exist). I guess it's unlikely that that might happen, but I felt it was more explicit to define a type.

Let me know if I misinterpreted your response.

I'm trying to figure out in what case would you call the datamanager and you need to differentiate a filenotfound via the datamanager vs a filenotfound at levels lower than the datamanager?

maybe can you show the before and the fixed version output in the PR description?

Currently, there is no case where a FileNotFound is raised lower than the data manager. However, since we leave open the possibility of custom data managers, it is possible some other future (3rd-party) implementation might throw a FileNotFound as a result of some internal error.

It would be completely possible to change the code to instead just work with the built-in FileNotFound exception, and as a result some of the code could be slightly simplified (mainly removing the few lines that store the pointer to the GKDataFileNotFoundException type). However, we would lose that slight granularity in determining whether a raised FileNotFound exception is a result of a (possible) data manager error, or a true file not found (data manager determined the file does not exist).

The consequence for losing that granularity is when a FileNotFound would be thrown, we are falling back to the old behaviour for this specific exception type (behaviour has been updated in PR description).

Do you feel one approach is preferable here?

genome_kit/gk_data.py

src/py_init.cpp

genome_kit/data_manager.py

ovesh · 2025-07-08T23:31:05Z

src/py_init.cpp

 	// Check for error
 	if (!new_path_obj) {
-		GK_THROW(value, "{}", traceback_format_exc_and_clear());
+		if (_gk_data_file_not_found_error && PyErr_ExceptionMatches(_gk_data_file_not_found_error)) {


Add a tests for both S3 and GCS: genome that doesn't exist, missing auth (specify a non-existing bucket name).

can probably replace this with a GK_RETHROW but not 100% sure anymore

Can't GKRETHROW here because the exception here originates from within python, which wont be caught by the rethrow catch blocks (since they're catching c++ exceptions).

genome_kit/gk_data.py

SophiaPerzan-DG · 2025-07-14T14:41:21Z

genome_kit/gk_data.py

+    if len(eps) > 0:
+        logging.warning("Failed to load the data manager plugin. "
+                        "Falling back to the default data manager.")


Previously if a custom data manager threw an error when attempting to load, GK would silently fallback to the default DM. Instead we warn the user.

SophiaPerzan-DG · 2025-07-14T14:42:00Z

genome_kit/gk_data.py

+    if len_eps > 1:
+        logging.info("Multiple data manager plugins found. "
+                     f"Using the first one: {list_eps[0].name}")


In the case of multiple data managers, only the first one is used. This behaviour would occur silently. Now we inform the user.

src/py_init.cpp

src/refg.cpp

tests/test_data_manager.py

tests/test_genome.py

tests/test_data_manager.py

ovesh · 2025-07-15T16:16:59Z

Please update the PR description with terminal output of the old and new behavior.

ovesh

Non-blocking comment, preemptively approving.

src/refg.cpp

fix: propagate errors when datamanager fails to resolve paths for a refg

6399a2f

SophiaPerzan-DG requested review from Copilot and ovesh June 23, 2025 20:10

Copilot AI reviewed Jun 23, 2025

View reviewed changes

src/refg.cpp Outdated Show resolved Hide resolved

src/refg.cpp Outdated Show resolved Hide resolved

src/refg.cpp Outdated Show resolved Hide resolved

SophiaPerzan-DG added 2 commits June 23, 2025 16:13

fix order of args in error msg

fe32e22

fix typo

44ae1f2

SophiaPerzan-DG closed this Jun 24, 2025

SophiaPerzan-DG added 4 commits June 25, 2025 14:47

chore: add custom exception type GKDataFileNotFoundError

bdabce7

chore: return GKDataFileNotFoundError when data manager fails to find…

9ae817c

… a file

chore: output logs when loading custom data manager

a8d4388

fix: detect when data manager fails and return appropriate error

0c57bf3

SophiaPerzan-DG reopened this Jul 8, 2025

ovesh requested changes Jul 8, 2025

View reviewed changes

SophiaPerzan-DG added 8 commits July 9, 2025 10:50

use python logging level

ef1a56e

dry references to entry points

f6e2fdb

add comment

7c641df

make message mandatory

e1464e5

chore: chain data manager error

c03e669

catch gk_data_file_not_found_error in related code

cbb38a0

test: add test to check for gk data file not found error raised

e82fa00

add test for non-existent genome

28731b0

SophiaPerzan-DG commented Jul 14, 2025

View reviewed changes

genome_kit/gk_data.py Outdated Show resolved Hide resolved

SophiaPerzan-DG commented Jul 14, 2025

View reviewed changes

ovesh reviewed Jul 14, 2025

View reviewed changes

ovesh requested a review from s22chan July 14, 2025 19:20

SophiaPerzan-DG added 2 commits September 15, 2025 10:48

chore: fix typo in error msg

42d48fd

chore: use exc_info to log debug error

21065ee

SophiaPerzan-DG added 3 commits September 15, 2025 11:43

chore: stylistic change to copy initialization for pointer

5fcef1f

chore: remove erroneous test skip in CI

fe8e543

add tests

3d5f318

SophiaPerzan-DG requested a review from ovesh September 16, 2025 16:03

ovesh approved these changes Sep 19, 2025

View reviewed changes

src/refg.cpp Outdated Show resolved Hide resolved

fix typo

f4ae02d

ovesh merged commit 00d61c7 into main Sep 23, 2025
11 checks passed

ovesh deleted the 171-datamanager-error-propagation branch September 23, 2025 16:17

github-actions bot mentioned this pull request Sep 16, 2025

chore(main): release 7.2.1 #183

Merged

	GKPY_CATCH_CASE(gk::assertion_error, PyExc_AssertionError, return_stmt) \
	GKPY_CATCH_CASE(gk::file_error, PyExc_OSError, return_stmt) \
	GKPY_CATCH_CASE(gk::type_error, PyExc_TypeError, return_stmt) \
	GKPY_CATCH_CASE(gk::value_error, PyExc_ValueError, return_stmt) \
	GKPY_CATCH_CASE(gk::index_error, PyExc_IndexError, return_stmt) \
	GKPY_CATCH_CASE(gk::key_error, PyExc_KeyError, return_stmt) \
	GKPY_CATCH_CASE(gk::memory_error, PyExc_MemoryError, return_stmt) \
	GKPY_CATCH_CASE(gk::not_implemented_error, PyExc_NotImplementedError, return_stmt) \
	GKPY_CATCH_CASE(gk::unreachable_code_error, PyExc_RuntimeError, return_stmt) \
	GKPY_CATCH_CASE(gk::runtime_error, PyExc_RuntimeError, return_stmt) \
	GKPY_CATCH_CASE(std::exception, PyExc_RuntimeError, return_stmt) \

fix: propagate errors when datamanager fails to resolve paths for a refg #172

fix: propagate errors when datamanager fails to resolve paths for a refg #172

Uh oh!

Conversation

SophiaPerzan-DG commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SophiaPerzan-DG commented Jun 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SophiaPerzan-DG Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s22chan Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s22chan Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ovesh commented Jul 15, 2025

Uh oh!

ovesh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SophiaPerzan-DG commented Jun 23, 2025 •

edited

Loading

SophiaPerzan-DG Jul 9, 2025 •

edited

Loading

s22chan Sep 15, 2025 •

edited

Loading

s22chan Sep 17, 2025 •

edited

Loading