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

Increase test coverage + Fix save_model_to_hdf5 + Improve is_remote_path + Fix is_remote_path #900

Merged

Conversation

Faisal-Alsrheed
Copy link
Contributor

@Faisal-Alsrheed Faisal-Alsrheed commented Sep 16, 2023

This PR aims to increase the robustness of the codebase by improving test coverage, fixing existing functions, and adding new tests to validate the changes made.

@codecov
Copy link

codecov bot commented Sep 16, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +7.04% 🎉

Comparison is base (74a4e7f) 76.78% compared to head (021068b) 83.83%.
Report is 56 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #900      +/-   ##
==========================================
+ Coverage   76.78%   83.83%   +7.04%     
==========================================
  Files         329      318      -11     
  Lines       31431    28731    -2700     
  Branches     6113     5486     -627     
==========================================
- Hits        24134    24086      -48     
+ Misses       5726     3140    -2586     
+ Partials     1571     1505      -66     
Flag Coverage Δ
keras_core 83.73% <100.00%> (+7.04%) ⬆️
keras_core-jax 67.17% <100.00%> (?)
keras_core-numpy 60.64% <100.00%> (?)
keras_core-tensorflow 66.97% <100.00%> (?)
keras_core-torch 69.23% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
keras_core/utils/file_utils.py 66.06% <100.00%> (+3.61%) ⬆️

... and 54 files with indirect coverage changes

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

@Faisal-Alsrheed Faisal-Alsrheed changed the title Increase test coverage in saving Increase test coverage Sep 16, 2023
@Faisal-Alsrheed Faisal-Alsrheed changed the title Increase test coverage [WIP] Increase test coverage Sep 17, 2023
@Faisal-Alsrheed Faisal-Alsrheed changed the title [WIP] Increase test coverage [WIP] Increase test coverage + Fix save_model_to_hdf5 + Improve is_remote_path + Fix is_remote_path Sep 18, 2023
@Faisal-Alsrheed Faisal-Alsrheed marked this pull request as ready for review September 18, 2023 17:09
@Faisal-Alsrheed Faisal-Alsrheed changed the title [WIP] Increase test coverage + Fix save_model_to_hdf5 + Improve is_remote_path + Fix is_remote_path Increase test coverage + Fix save_model_to_hdf5 + Improve is_remote_path + Fix is_remote_path Sep 18, 2023
Copy link
Contributor

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

keras_core/callbacks/lambda_callback_test.py Outdated Show resolved Hide resolved
keras_core/callbacks/lambda_callback_test.py Outdated Show resolved Hide resolved
keras_core/callbacks/lambda_callback_test.py Outdated Show resolved Hide resolved
keras_core/callbacks/lambda_callback_test.py Outdated Show resolved Hide resolved
keras_core/saving/saving_api_test.py Outdated Show resolved Hide resolved
keras_core/utils/file_utils.py Outdated Show resolved Hide resolved

# Specific patterns for supported remote paths
supported_patterns = [
re.compile(r"^(gs|cns|cfs|http|https|ftp|s3)://.*$", re.IGNORECASE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Which ones of these work with gfile? Since we use gfile for remote paths, we should only match the paths for which it will work (otherwise using local accessors which will raise an error is fine).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WIP

Copy link
Contributor Author

@Faisal-Alsrheed Faisal-Alsrheed Sep 20, 2023

Choose a reason for hiding this comment

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

Hello @fchollet,

I've refined the is_remote_path function to only recognize gs:// and hdfs:// based on gfile capabilities.

I've also added a warning message when encountering an unrecognized remote path. This provides immediate feedback to developers, helping them quickly identify and address potential issues.

Would appreciate your feedback on this approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

def is_remote_path(filepath):
    filepath_str = str(filepath).strip()
    supported_patterns = [re.compile(r"^(gs|hdfs)://.*$", re.IGNORECASE)]

    for pattern in supported_patterns:
        if pattern.match(filepath_str):
            return True

    # Log or print the error message without raising an exception
    warning_msg = (
        f"Warning: The path '{filepath_str}' is not recognized as a "
        f"supported remote path by gfile. Supported paths are: "
        f"{', '.join(['gs://', 'hdfs://'])}"
    )
    print(warning_msg)

    return False

Copy link
Contributor

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

LGTM -- thanks!

@Faisal-Alsrheed
Copy link
Contributor Author

Faisal-Alsrheed commented Sep 19, 2023

@fchollet
Thank you for your feedback.
I am still working on your comments.
I will will let you know once it is ready

@Faisal-Alsrheed Faisal-Alsrheed changed the title Increase test coverage + Fix save_model_to_hdf5 + Improve is_remote_path + Fix is_remote_path [WIP] Increase test coverage + Fix save_model_to_hdf5 + Improve is_remote_path + Fix is_remote_path Sep 19, 2023
@Faisal-Alsrheed Faisal-Alsrheed changed the title [WIP] Increase test coverage + Fix save_model_to_hdf5 + Improve is_remote_path + Fix is_remote_path Increase test coverage + Fix save_model_to_hdf5 + Improve is_remote_path + Fix is_remote_path Sep 20, 2023
Copy link
Contributor

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

Thank you -- great work!

@fchollet fchollet merged commit a354797 into keras-team:main Sep 20, 2023
8 checks passed
@Faisal-Alsrheed Faisal-Alsrheed deleted the Increase-test-coverage-in-`saving`-v2 branch September 21, 2023 06:18
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.

2 participants