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

Fix huggingface inference endpoint name #1011

Merged
merged 3 commits into from
Nov 19, 2024

Conversation

jmartin-tech
Copy link
Collaborator

fix #998

the name provided during construction is populated by the call to super().__init__(), access to self attributes is required for any value populated by Configurable.

Verification

List the steps needed to make sure this thing works

  • Execute against public InferenceAPI:
python -m garak --model_type huggingface.InferenceAPI --model_name microsoft/Phi-3-mini-4k-instruct --probes malwaregen.Evasion
  • Verify valid responses are received from endpoint.
  • Verify added automation tests pass

the name provided during construction is populated by
the call to `super().__init__()`, access to `self` attributes is required
for any value populated by `Configurable`.

Signed-off-by: Jeffrey Martin <[email protected]>
Add validation test to ensure uri values and names populate as expected

Signed-off-by: Jeffrey Martin <[email protected]>
Copy link
Collaborator

@erickgalinkin erickgalinkin left a comment

Choose a reason for hiding this comment

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

LGTM. Did we test on an actual HF InferenceAPI just to be sure it works as intended? If not, I can spin one up tomorrow.

@jmartin-tech
Copy link
Collaborator Author

I tested the public endpoint referenced in the issue, I have not done an end to end test with a private endpoint.

@ppietikainen
Copy link

ppietikainen commented Nov 19, 2024

Tested the fix and works for what we've been trying out. Also
python3 -m garak --model_type huggingface.InferenceEndpoint --model_name https://api--inference.huggingface.co/models/microsoft/Phi-3-mini-4k-instruct --probes malwaregen.Evasion
seems to do queries to the right place, so I would assume private ones would work too.

I guess (and seemed to work, tho it's getting late)

def __init__(name="")
  super().__init__(name, config_root=config_root)
  self.uri = ... + self.name

in both would make it a bit more easy to follow, the self.name = name confused me when trying to figure the code out, as I thought it implies the super() won't do anything to it :-)

@jmartin-tech
Copy link
Collaborator Author

jmartin-tech commented Nov 19, 2024

@ppietikainen thanks for the extra testing, we have tried to add documentation on how Configurable classes config values are expected to be prioritized. The assignment of name as provided by to the constructor relates to how the precedence of values are treated allowing constructor values to be held above configuration file values that are injected by super().

Also, in the different classes name has competing usage. For InferenceAPI it is expected to just be the model name and gets added to the public base uri defined in a constant, in InferenceEndpoint the full uri must be supplied. I do think there may be some consistency to be gained at some point in the future. I don't have a chosen path forward in mind so for now this can land to at least return the functionality to original state before the regression.

@jmartin-tech jmartin-tech merged commit c7a9fa6 into NVIDIA:main Nov 19, 2024
9 checks passed
@jmartin-tech jmartin-tech deleted the fix/hf-endpoint-name branch November 19, 2024 21:22
@github-actions github-actions bot locked and limited conversation to collaborators Nov 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Huggingface InferenceAPI stopped working after 0.9.0.13 due to name parameter not being set
3 participants