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

API for topics #1023

Merged
merged 3 commits into from
Mar 9, 2021
Merged

API for topics #1023

merged 3 commits into from
Mar 9, 2021

Conversation

sahithyaravi
Copy link
Member

@sahithyaravi sahithyaravi commented Feb 15, 2021

Reference Issue

What does this PR implement/fix? Explain your changes.

Topic APIs - For adding topics for datasets based on a topic model

  • topic_add - to add a topic to a dataset
  • topic_delete - to delete a topic added to a dataset

We are going to use this in the new website to group datasets/filter datasets by topic.

This API can be accessed only by admin users.
In future, we will add a get API to get topics per dataset or list datasets by topic.
.

How should this PR be tested?

Any other comments?

  • Since the API can only be used by admin, I tested this locally with the admin key to see if it works.
  • The unit test added only checks if an error occurs when a non-admin user accesses these APIs.
  • For the same reason I did not add an example. Please let me know if I need to add an example (With a note to fill in admin key before usage for instance)

Copy link
Collaborator

@PGijsbers PGijsbers left a comment

Choose a reason for hiding this comment

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

I think these functions should be marked private (i.e. underscore prefix and not imported/added to __all__ in __init__.py to avoid accidental imports). This allows the code itself to signal to users that it shouldn't be used by them.

Not writing an example is fine, given we don't expect a regular user to use this functionality. However, for later reference it might be good to add a section to the api documentation with admin-only functions.

I would like @mfeurer to weigh in on this though. What are your thoughts on how to add admin-only operations? The main alternative I considered would be to perhaps add a separate submodule for them, but as long as there aren't many of them this makes more sense to me.

@mfeurer
Copy link
Collaborator

mfeurer commented Feb 17, 2021

I would like @mfeurer to weigh in on this though. What are your thoughts on how to add admin-only operations? The main alternative I considered would be to perhaps add a separate submodule for them, but as long as there aren't many of them this makes more sense to me.

I don't have a strong opinion on this, but it looks like it would be a bit nicer in a separate admins package.

@PGijsbers
Copy link
Collaborator

Okay, in that case I propose to just go with the private methods for now, and refactor to an admin package when we have more admin features.

@PGijsbers PGijsbers merged commit 6c609b8 into develop Mar 9, 2021
@PGijsbers PGijsbers deleted the topic_apis branch March 9, 2021 10:42
github-actions bot pushed a commit that referenced this pull request Mar 9, 2021
PGijsbers pushed a commit to Mirkazemi/openml-python that referenced this pull request Feb 23, 2023
* initial commit

* private functions
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.

3 participants