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

PEPhubclient should correctly handle domains for registry paths #48

Open
nsheff opened this issue Aug 22, 2024 · 5 comments
Open

PEPhubclient should correctly handle domains for registry paths #48

nsheff opened this issue Aug 22, 2024 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@nsheff
Copy link

nsheff commented Aug 22, 2024

If I make a looper config file with a weird, bogus registry path like this:

pep_config: bogus-website.com://databio/pep_derived_attrs:default
output_dir: results
pipeline_interfaces:
  - pipeline/pipeline_interface.yaml

It is actually ignoring the domain information, and just grabbing the databio/pep_derived_attrs:default section, and retrieving it from pephub, and running the pipeline correctly.

This means we're not handling the domain right. We're only allowing pephub.databio.org, regardless of what you put in the domain. This means we can't allow people to use this with a registry other than ours.

Instead, we can default to the main interface, but we need to allow the user to specify an alternative domain. What if I want to try this on a local instance, using localhost://registry_path? This is not currently possible.

Not sure if this is something to be fixed in looper or in PEPhubClient.

@nsheff nsheff added the bug Something isn't working label Aug 22, 2024
@donaldcampbelljr
Copy link

https://github.com/pepkit/looper/blob/5bdcdc64b6b8ba80344a8b86d7fb2d1a1838c546/looper/cli_pydantic.py#L212-L226

https://github.com/pepkit/looper/blob/5bdcdc64b6b8ba80344a8b86d7fb2d1a1838c546/looper/utils.py#L719-L729

Currently, looper checks if the registry path can be parsed using ubiquerg and checks that a RegistryPath object from PEPHubclient can be created. If so, the path is then used to create a project via PepHubClient using that registry path.

@khoroshevskyi
Copy link
Member

Right now, you can specify PEPhub domain using env vars. More info here: https://pep.databio.org/pephub/developer/pephubclient/#how-to-specify-url-for-pephub-instance

@nsheff nsheff transferred this issue from pepkit/looper Aug 27, 2024
@nsheff nsheff changed the title Looper should correctly handle domains for registry paths PEPhubclient should correctly handle domains for registry paths Aug 27, 2024
@khoroshevskyi
Copy link
Member

khoroshevskyi commented Aug 28, 2024

I found the issue that I mentioned earlier about parsing registry path with url.
If we want to parse string with protocol. e.g."https://bogus-website.com://databio/pep_derived_attrs:default", then we have 2 separators of protocol: https :// bogus-website.com :// and it leads to incorrect parsing with ubiquerg.

At this point I see 2 solutions of this problem:

  1. Assume that all domains will have https protocol. (Then localhost won't work)
  2. Try to write a new parsing function without using ubiquerg

Any other way to solve this problem?

@nsheff
Copy link
Author

nsheff commented Aug 28, 2024

Actually I think we should use https://bogus-website.com/databio/pep_derived_attrs:default

there should just be a /, not 2 of ://.

but this is also not correctly parsed.

import ubiquerg
ubiquerg.parse_registry_path("bogus-website.com/databio/pep_derived_attrs")

the function is parsing as {protocol}://{namespace}/{item}.{subitem}:{tag}.

I think we want to add a third layer in there, which would be domain.

Can you update that function to allow domains, like this?

{protocol}://{domain}/{namespace}/{item}.{subitem}:{tag}.

@nsheff
Copy link
Author

nsheff commented Aug 28, 2024

also right now that function is silently failing, when it can't get anything. should it raise an exception? or return a dict with None in each entry if it can't parse anything?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants