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

do not require last statement to be an expression or an instance of DatasetQuery #395

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

skshetry
Copy link
Member

@skshetry skshetry commented Sep 5, 2024

This PR removes the requirement for the last statement to be either an expression or an instance of DatasetQuery.

After this PR, scripts without a final expression will function properly in both Datachain and Studio.

The last statement wrappers and related infrastructure will be removed in subsequent PRs.
However, this PR demonstrates that everything already works and is usable.

from datachain.query import DatasetQuery
DatasetQuery("s3://dql-test-e2e", catalog=catalog).save("temp")
print("hello world!")
$ datachain query script.py
Processed: 1 rows [00:00, 512.00 rows/s]
Generated: 9 rows [00:00, 4950.00 rows/s]
Cleanup: 1 tables [00:00, 3498.17 tables/s]
Saving: 279 rows [00:00, 68945.43 rows/s]
hello world!

Part of #360.

@@ -971,42 +971,6 @@ def test_query_subprocess_wrong_return_code(mock_popen, cloud_test_catalog):
assert str(exc_info.value).startswith("Query script exited with error code 1")


def test_query_last_statement_not_expression(mock_popen, cloud_test_catalog):
Copy link
Member Author

@skshetry skshetry Sep 5, 2024

Choose a reason for hiding this comment

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

These tests are mocked tests. And since these tests are no longer a failure, they are not useful.

(There is a real test in test_query.py below).

@skshetry skshetry force-pushed the relax-last-statement-req branch from 7098d80 to 5ccdef3 Compare September 5, 2024 10:51
Copy link

cloudflare-workers-and-pages bot commented Sep 5, 2024

Deploying datachain-documentation with  Cloudflare Pages  Cloudflare Pages

Latest commit: 638bafe
Status: ✅  Deploy successful!
Preview URL: https://13ce0857.datachain-documentation.pages.dev
Branch Preview URL: https://relax-last-statement-req.datachain-documentation.pages.dev

View logs

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.72%. Comparing base (a8d3640) to head (638bafe).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #395      +/-   ##
==========================================
- Coverage   86.82%   86.72%   -0.11%     
==========================================
  Files          93       93              
  Lines       10043    10040       -3     
  Branches     2045     2044       -1     
==========================================
- Hits         8720     8707      -13     
- Misses        980      987       +7     
- Partials      343      346       +3     
Flag Coverage Δ
datachain 86.67% <100.00%> (-0.11%) ⬇️

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.

Copy link
Contributor

@dreadatour dreadatour left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you! 🙏

We also need to make sure it all works on Studio side (including frontend part).

@skshetry
Copy link
Member Author

skshetry commented Sep 5, 2024

Looks good to me, thank you! 🙏

We also need to make sure it all works on Studio side (including frontend part).

It does work well on Studio. We might need to adjust some stuff like "Register dataset" button which is no longer possible as we need explicit saves.

But I'd not consider this as completely functional, that will happen when we completely remove this wrapping thing.

@dreadatour
Copy link
Contributor

Thank you for handling this ❤️

@skshetry skshetry force-pushed the relax-last-statement-req branch from 5ccdef3 to 638bafe Compare September 5, 2024 11:07
@skshetry skshetry merged commit a9f77ba into main Sep 5, 2024
33 of 38 checks passed
@skshetry skshetry deleted the relax-last-statement-req branch September 5, 2024 15:01
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.

2 participants