Skip to content

Conversation

@Inokinoki
Copy link
Member

@Inokinoki Inokinoki commented Nov 1, 2023

Description

Related Issue

[GSK-1990]
[GSK-1509]

Type of Change

  • 📚 Examples / docs / tutorials / dependencies update
  • 🔧 Bug fix (non-breaking change which fixes an issue)
  • 🥂 Improvement (non-breaking change which improves an existing feature)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 🔐 Security fix

Checklist

  • I've read the CODE_OF_CONDUCT.md document.
  • I've read the CONTRIBUTING.md guide.
  • I've updated the code style using make codestyle.
  • I've written tests for all new methods and classes that I created.
  • I've written the docstring in Google format for all the methods and classes that I used.

@linear
Copy link

linear bot commented Nov 1, 2023

GSK-1509 Add integration tests for WebSocket communication

  1. Java side integration test

    create a WebSocket endpoint, use a fake WebSocket connection to connect and test

  2. Python-Java integration test

    replace callbacks with mock function, use test-container to create a backend. use ~requests~ to call Web interface, then assert the messages received

  3. Python integration test

    how to test the implementations in listener.py ? - (testing with tests.utils.MockedClient should work)

@Inokinoki Inokinoki changed the title [GSK-1509] Add tests for downloading [GSK-1509] Add tests for download and upload Nov 1, 2023
Inokinoki added a commit that referenced this pull request Nov 1, 2023
@Inokinoki Inokinoki changed the title [GSK-1509] Add tests for download and upload [GSK-1509] Add up/download tests for model, dataset and callable Nov 1, 2023
@Inokinoki Inokinoki changed the title [GSK-1509] Add up/download tests for model, dataset and callable [GSK-1509][GSK-1990] Add up/download tests for model, dataset and callable Nov 1, 2023
@linear
Copy link

linear bot commented Nov 1, 2023

GSK-1990 Add pydantic config (ie extra = forbid) to all dtos

To avoid any silent typo or error between server and worker, and also client and server, pydantic object should be using some configuration.

@dataclass
class DatasetMeta:
name: Optional[str]
target: str
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be target: Optional[str] = None ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi Andrey, thanks very much for the review!

The Optional[str] without default value means that we have to provide it in constructor, but it could be None:

>>> from giskard.core.core import DatasetMeta
>>> DatasetMeta()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: DatasetMeta.__init__() missing 6 required positional arguments: 'name', 'column_types', 'column_dtypes', 'number_of_rows', 'category_features', and 'target'

while having the default value means that we do not have to provide it:

>>> from giskard.core.core import DatasetMeta
>>> DatasetMeta()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: DatasetMeta.__init__() missing 5 required positional arguments: 'name', 'column_types', 'column_dtypes', 'number_of_rows', and 'category_features'

In the DatasetMeta case, all of the information should have been provided by the Dataset class, but the target could be None now with LLM model.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@andreybavt andreybavt 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, I tested on MozartBiographer.ipynb and it worked

@andreybavt andreybavt merged commit dd11d92 into main Nov 2, 2023
@andreybavt andreybavt deleted the feature/gsk-1509-add-tests-for-downloading branch November 2, 2023 16:48
Inokinoki added a commit that referenced this pull request Nov 3, 2023
Hartorn added a commit that referenced this pull request Nov 6, 2023
* Add unit test for echo action in `listener.py`

* Add a test to ensure the registered actors

* Move echo actor test into `test_websocket_actor`

* Add mocked ML worker for ML worker info generation

* Add test for `getInfo` actor

* Fix `None` when installed packages not required

* Add test for `stopWorker` actor

* Add test for `get_catalog` actor

* Fix pdm lint due to codestyle

* Add `runModel` actor for internal worker

* Add unit tests for model running on dataframe

* Use utils to save model and dataset in tests

* Add unit tests for `explain`, `explainText` actors

* Fix type in parameters for model inputs

* Fix WebSocket dataframe arguments creation

* Make `GiskardClient` nullable in WebSocket params

* Add a unit test to data processing without func

* Add workaround and use empty list

* Create utils to get the cache path

* Add tests for slice and transformation func upload
Using a mock client and check local cache

* Unify the constants of slice, transformation func

* Unify unit tests for callable function uploading

* Unit tests for savable func download (no project)

* Add similar tests for callable download in project

* Improve loading from module in unit tests

* Improve URL gen in downloading tests

* Add util to save callables under home cache

* Move meta info for callables fixup to `utils.py`

* Add single-function test case for data process

* Fix missing dataset mock data fixup util

* Simplify data processing actor tests

* Add tests for dataset downloading

* Add tests for model downloading

* Add project cache guard for testing

* Add temp cache guard for websocket actor tests

* Add test for external worker runModel action

* Add test for runModelForDataFrame actor in external worker

* Add external worker tests for explaination actors

* Add external worker dataset processing test

* Clean up

* Rename tests for callables downloading

* Fixup missing fields in mock dataset meta info

* Fix internal worker log artifact local

* Remove unused posixpath

* Fix tests using mock dataset

* Fix and improve dataset util

* Add tests for run adhoc tests w/o debug

* Separate tests

* Add tests for running test suites

* Fix meta info loading in internal ML worker

* Remove auto-created `version` from mandatory fields

* Remove `generateTestSuite` actor

* Migrate artifact downloading tests with new utils

* Add more utils to test artifact download

* Add test for loading from local in internal worker

* Remove imports for `generateTestSuite`

* Improve local artifact logging

* Remove changes already done in #1535

* Remove unused Path from pathlib

* Sync `utils.py` from #1535

* Add tests for abnormal execution path

* Add tests for invalid push kind in push feature

* Add tests for push without call-to-action

* Add tests for contribution push with CTA

* Add tests for perturbation push with CTA

* Add tests for borderline push with CTA

* Add tests for overconfidence push with CTA save example

* Add comments for test related CTA in push feature
`CREATE_TEST`, `ADD_TEST_TO_CATALOG`

* Fix test for ad-hoc running with no name dataset

* Clean cache before each push feature test

* Remove tests for INVALID push kind

* Fix typo in saving perturbation from push

---------

Co-authored-by: Hartorn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants