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

Remove limit on number of results returned from get_filter_values(). #21

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

aaronweeden
Copy link

@aaronweeden aaronweeden commented Nov 28, 2023

Description

This PR removes the limit on the number of results returned from the get_filter_values() method. The limit was arbitrary and unnecessary.

Tests performed

This PR adds a new regression test to make sure more than 10,000 (the previous limit) results are returned.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • CHANGELOG.md has been updated
  • The milestone is set correctly on the pull request
  • The appropriate labels have been added to the pull request
  • Running the automated tests (see docs/developing.md) produces no errors
  • Updates have been made to the xdmod-notebooks repository as necessary, and the notebooks all run successfully

@aaronweeden aaronweeden added the bug Something isn't working label Nov 28, 2023
@aaronweeden aaronweeden added this to the 1.0.1 milestone Nov 28, 2023
@connersaeli
Copy link

I am having an issue with tests/regression/test_datawarehouse_regression.py::test_describe_realms. Data for that test seems to be edited in #14 to remove:

ResourceAllocations, Resource Allocations

However when I use the describe_realms() API call against production XDMoD, it appears that these lines are still present in the returned DataFrame. The test passes again once I add that line back into tests/regression/data/realms.csv. I can reproduce this for both main and this PRs branches. It could have been an oversight in my review and those lines might just need to be added back in.

It is probably out of scope for this PR, but I just wanted to mention with that being the only test breaking for this.

Copy link

@connersaeli connersaeli left a comment

Choose a reason for hiding this comment

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

I was able to reinstall this branch of xdmod-data in a JupyterLab server. I already had a Notebook running that previously failed to find a specific filter for an Allocation. After reinstalling, I restarted the kernel and successfully managed to grab the information for the allocation.

I also ran all of the available tests. Specifically, the newly added test_get_data_filter_user() passed.

@aaronweeden
Copy link
Author

I am having an issue with tests/regression/test_datawarehouse_regression.py::test_describe_realms. Data for that test seems to be edited in #14 to remove:

ResourceAllocations, Resource Allocations

However when I use the describe_realms() API call against production XDMoD, it appears that these lines are still present in the returned DataFrame. The test passes again once I add that line back into tests/regression/data/realms.csv. I can reproduce this for both main and this PRs branches. It could have been an oversight in my review and those lines might just need to be added back in.

It is probably out of scope for this PR, but I just wanted to mention with that being the only test breaking for this.

@connersaeli thanks for reviewing, ResourceAllocations will appear if you don't have "User" set as your Top Role like it says to do in the testing instructions.

@aaronweeden aaronweeden merged commit a046f6e into ubccr:main Apr 5, 2024
1 check passed
@aaronweeden aaronweeden deleted the fix-filters-10000 branch April 5, 2024 20:00
@connersaeli
Copy link

@aaronweeden That was the issue thanks! Sorry about that I elevated my Top Role for something else after I last tried this.

@aaronweeden
Copy link
Author

@aaronweeden That was the issue thanks! Sorry about that I elevated my Top Role for something else after I last tried this.

@connersaeli No problem, thanks again for reviewing.

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

Successfully merging this pull request may close these issues.

2 participants