Skip to content

Conversation

@mdambski
Copy link
Contributor

@mdambski mdambski commented Feb 8, 2025

Description

This change set introduces the sha256 hashing option as an alternative to md5. By default, the library will use md5 unless this hashing algorithm is unavailable (which would effectively break the library for the user).

Rationale

At the moment, the library uses md5 hashing by default and does not allow the option to choose an alternative.

In some distributions of Python that are compliant with FIPS Regulation, md5 will either be missing or will raise an error when used. These concerns are brought up in the Python docs related to hash algorithms.

In such environments, using a library that relies solely on md5 forces users to either abandon usage of the library or create and maintain their own fork with the needed patches. Both options are far from optimal.

cc: @Pouyanpi

Related Issue(s)

Checklist

  • I've read the CONTRIBUTING guidelines.
  • I've updated the documentation if applicable.
  • I've added tests if applicable.
  • @mentions of the person or team responsible for reviewing proposed changes.

Copy link
Collaborator

@Pouyanpi Pouyanpi 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 very much @mdambski, the PR looks great, but I think we can make it simpler as the hashing in kb is not that critical.

Also, I think when we allow the users to configure the caching there is no need to set the default dynamically.

please have a look at my comments and let me know your thoughts, It could be that I've missed the actual purpose. Thanks!

@github-actions
Copy link
Contributor

Documentation preview

https://nvidia.github.io/NeMo-Guardrails/review/pr-988

@mdambski mdambski requested a review from Pouyanpi February 11, 2025 15:48
@mdambski mdambski changed the title Feat: Add SHA-256 Hashing Option and Configuration Capabilities Feat: Add SHA-256 hashing option Feb 11, 2025
@mdambski
Copy link
Contributor Author

Hi @Pouyanpi, all change requests are addressed. Please let me know if we can move forward 🙏

@Pouyanpi
Copy link
Collaborator

Thank you @mdambski you just need to remove the unused config folder

@mdambski
Copy link
Contributor Author

mdambski commented Feb 21, 2025

@Pouyanpi the config was removed

@mdambski mdambski requested a review from Pouyanpi February 21, 2025 12:35
Copy link
Collaborator

@Pouyanpi Pouyanpi left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me.

Copy link
Collaborator

@Pouyanpi Pouyanpi left a comment

Choose a reason for hiding this comment

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

Tests are failing for Python 3.9 (test_utils.py)

@mdambski would you please have a look?

@Pouyanpi Pouyanpi self-requested a review February 21, 2025 18:54
@Pouyanpi Pouyanpi added this to the v0.13.0 milestone Feb 23, 2025
@mdambski
Copy link
Contributor Author

@Pouyanpi the tests are fixed 👍

@Pouyanpi
Copy link
Collaborator

@mdambski Thank you very much. I'll merge this PR later as it is decided to be part of 0.13.0 release. Probably Python 3.12 also needs to wait for 0.13.0, I'll let you know.

@Pouyanpi Pouyanpi merged commit 0dc77ff into NVIDIA-NeMo:develop Feb 27, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feature: Add FIPS compliant hashing algorithm SHA256

2 participants