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

SNOW-1620446: add timedelta integration tests and notebooks #2142

Merged

Conversation

sfc-gh-lmukhopadhyay
Copy link
Contributor

  1. Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-1620446

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
      • If this test skips Local Testing mode, I'm requesting review from @snowflakedb/local-testing
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency
    • If this is a new feature/behavior, I'm adding the Local Testing parity changes.
  3. Please describe how your code solves the related issue.

Adding notebooks with timedelta usecases and negative integration tests

Signed-off-by: Labanya Mukhopadhyay <labanya.mukhopadhyay@snowflake.com>
@sfc-gh-lmukhopadhyay sfc-gh-lmukhopadhyay added NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md NO-PANDAS-CHANGEDOC-UPDATES This PR does not update Snowpark pandas docs labels Aug 21, 2024
@sfc-gh-lmukhopadhyay sfc-gh-lmukhopadhyay marked this pull request as ready for review August 22, 2024 00:58
@sfc-gh-lmukhopadhyay sfc-gh-lmukhopadhyay requested a review from a team as a code owner August 22, 2024 00:58
@sfc-gh-evandenberg
Copy link
Collaborator

Are there any positive tests here?

@sfc-gh-nkrishna
Copy link
Contributor

@sfc-gh-lmukhopadhyay Binary op subtraction (datetime - datetime = timedelta) was merged recently. Can you ensure this is part of the tests in this PR?

Copy link
Collaborator

@sfc-gh-azhan sfc-gh-azhan left a comment

Choose a reason for hiding this comment

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

Is there a readme describe how to run notebook tests locally? Like how to setup the credentials for example?

@sfc-gh-lmukhopadhyay
Copy link
Contributor Author

Are there any positive tests here?

@sfc-gh-evandenberg The notebooks have some positive tests, but the idea is to re-enable the negative tests in test_timedelta_ops.py and undo the commented cells in the notebooks once all Timedelta operations are implemented, as outlined in this ticket here

@sfc-gh-lmukhopadhyay
Copy link
Contributor Author

Is there a readme describe how to run notebook tests locally? Like how to setup the credentials for example?

@sfc-gh-azhan The credentials should already be setup, provided you've populated your tests.parameters file with CONNECTION_PARAMETERS. The notebooks can then be run by running jupyter notebook in this repo and opening the notebook files to run the cells.
All the notebooks can also be run automatically via python -m pytest -ra --nbmake --nbmake-timeout=1000 tests/notebooks/modin which is how the daily notebook test is run. There isn't a readme for this yet but I can add one if it'd needed.

@sfc-gh-azhan
Copy link
Collaborator

Is there a readme describe how to run notebook tests locally? Like how to setup the credentials for example?

@sfc-gh-azhan The credentials should already be setup, provided you've populated your tests.parameters file with CONNECTION_PARAMETERS. The notebooks can then be run by running jupyter notebook in this repo and opening the notebook files to run the cells. All the notebooks can also be run automatically via python -m pytest -ra --nbmake --nbmake-timeout=1000 tests/notebooks/modin which is how the daily notebook test is run. There isn't a readme for this yet but I can add one if it'd needed.

Please add one in the notebook test folder thanks!

Signed-off-by: Labanya Mukhopadhyay <labanya.mukhopadhyay@snowflake.com>
Signed-off-by: Labanya Mukhopadhyay <labanya.mukhopadhyay@snowflake.com>
@sfc-gh-lmukhopadhyay
Copy link
Contributor Author

@sfc-gh-lmukhopadhyay Binary op subtraction (datetime - datetime = timedelta) was merged recently. Can you ensure this is part of the tests in this PR?

@sfc-gh-nkrishna This case should be in the MIMIC nb but is also explicitly in TimeSeriesTesting.ipynb now

@sfc-gh-lmukhopadhyay sfc-gh-lmukhopadhyay enabled auto-merge (squash) August 22, 2024 23:38
@sfc-gh-lmukhopadhyay sfc-gh-lmukhopadhyay merged commit 85aaf39 into main Aug 22, 2024
34 checks passed
@sfc-gh-lmukhopadhyay sfc-gh-lmukhopadhyay deleted the lmukhopadhyay-SNOW-1620446-timedelta-integ-tests branch August 22, 2024 23:56
@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md NO-PANDAS-CHANGEDOC-UPDATES This PR does not update Snowpark pandas docs snowpark-pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants