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

Introduce querying capabilities to fetch_runs_table #1660

Merged
merged 16 commits into from Mar 6, 2024
Merged

Conversation

szysad
Copy link
Contributor

@szysad szysad commented Feb 20, 2024

Before submitting checklist

  • Did you update the CHANGELOG? (not for test updates, internal changes/refactors or CI/CD setup)
  • Did you ask the docs owner to review all the user-facing changes?

Copy link

codecov bot commented Feb 20, 2024

Codecov Report

Attention: Patch coverage is 85.48387% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 77.10%. Comparing base (c84f454) to head (7bb41d7).
Report is 5 commits behind head on master.

Files Patch % Lines
src/neptune/metadata_containers/utils.py 50.00% 5 Missing ⚠️
src/neptune/metadata_containers/project.py 84.61% 2 Missing ⚠️
src/neptune/api/searching_entries.py 88.88% 1 Missing ⚠️
src/neptune/internal/backends/nql.py 96.15% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1660      +/-   ##
==========================================
- Coverage   80.58%   77.10%   -3.48%     
==========================================
  Files         302      302              
  Lines       15484    15538      +54     
==========================================
- Hits        12478    11981     -497     
- Misses       3006     3557     +551     
Flag Coverage Δ
e2e-management ?
e2e-s3 ?
e2e-s3-gcs ?
e2e-standard ?
macos 74.27% <85.48%> (-5.94%) ⬇️
py3.7 74.59% <85.48%> (-5.35%) ⬇️
ubuntu 74.46% <85.48%> (-5.84%) ⬇️
unit 74.59% <85.48%> (-0.40%) ⬇️
windows 73.51% <85.48%> (-6.41%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@szysad szysad marked this pull request as ready for review February 27, 2024 11:16
@szysad szysad changed the title Introduce querying capabilities to fetch_*_table Introduce querying capabilities to fetch_runs_table Feb 27, 2024
src/neptune/metadata_containers/project.py Outdated Show resolved Hide resolved
src/neptune/metadata_containers/project.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
assert len(runs_table) == 1
assert runs_table[0].get_attribute_value("sys/id") == run_id1
if with_query:
kwargs = {"query": f"(sys/tags: stringSet CONTAINS '{tag1}')"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the UI, CONTAINS is not available for tags. For tags, owner, state, and stage we have ONE OF, but I guess that's not supported API-side. Are we OK with such inconsistency? Are the differences outlined anywhere?

To document this properly, I'd need to understand exactly how the query syntax compares to we have here: https://docs.neptune.ai/app/searching_table/#syntax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@normandy7

My best bet is that ONE OF is translated in the frontend to expression that is using CONTAINS instead (I didn't confirm that with anyone).
If you take a look at nql grammar definition there is no ONE OF keyword so there is no way to run raw queries (which we are enabling) that are using this type of expression.

Copy link
Contributor Author

@szysad szysad Feb 28, 2024

Choose a reason for hiding this comment

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

UPDATE @normandy7 :
Yep thats what I thought. There is frontend test that confirms that

So basically there is backend NQL syntax, and frontend NQL syntax. Backend NQL does not contain ONE OF expression but frontend NQL does and is later on translated to backend NQL syntax which gets rid of ONE OF using CONTAINS.

We are running queries against backend so we have to use NQL which does not contain ONE OF expression (and is not accurately documented). So if we are not going to change backend NQL there is no way around this difference. (We might do similar translation thats happening in frontend but thats out of scope for raw string queries)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for finding out!

That's a bummer, though. I suppose we have to live with this, but from an end-user POV it can be annoying/confusing. CC: @parthpankajtiwary

CHANGELOG.md Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
src/neptune/api/utils.py Outdated Show resolved Hide resolved
src/neptune/common/warnings.py Outdated Show resolved Hide resolved
src/neptune/exceptions.py Outdated Show resolved Hide resolved
src/neptune/metadata_containers/utils.py Outdated Show resolved Hide resolved
src/neptune/internal/backends/nql.py Show resolved Hide resolved
Szymon Sadkowski and others added 11 commits March 4, 2024 09:30
- add RawNQLQuery which represents raw string query
- add eval() method to all NQLQuery classes which returns simplified version of type
…lity

- all test that use fetch_runs_tables now are parametrized to include query (even trival cases) in one of the cases
- new test are added
Co-authored-by: Sabine <sabine.nyholm@neptune.ai>
Co-authored-by: Sabine <sabine.nyholm@neptune.ai>
@szysad szysad requested review from Raalsky and normandy7 March 4, 2024 09:36
src/neptune/exceptions.py Outdated Show resolved Hide resolved
Co-authored-by: Sabine <sabine.nyholm@neptune.ai>
@szysad szysad merged commit c64dbf4 into master Mar 6, 2024
4 checks passed
@szysad szysad deleted the ss/fetch-ui-parity branch March 6, 2024 15:43
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.

None yet

3 participants