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

improve help options #60

Merged
merged 17 commits into from
Jul 28, 2021
Merged

improve help options #60

merged 17 commits into from
Jul 28, 2021

Conversation

satra
Copy link
Member

@satra satra commented Jul 15, 2021

This PR improves several of the items that show up in the metadata editor and elsewhere.

@satra satra requested a review from bendichter July 15, 2021 16:58
@codecov
Copy link

codecov bot commented Jul 15, 2021

Codecov Report

Merging #60 (9e8050b) into master (ccea009) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   96.07%   96.14%   +0.07%     
==========================================
  Files          13       13              
  Lines        1274     1298      +24     
==========================================
+ Hits         1224     1248      +24     
  Misses         50       50              
Flag Coverage Δ
unittests 96.14% <100.00%> (+0.07%) ⬆️

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

Impacted Files Coverage Δ
dandischema/consts.py 100.00% <100.00%> (ø)
dandischema/metadata.py 96.79% <100.00%> (ø)
dandischema/models.py 94.11% <100.00%> (+0.19%) ⬆️
dandischema/tests/test_models.py 95.71% <100.00%> (+0.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccea009...9e8050b. Read the comment docs.

Copy link
Member

@bendichter bendichter left a comment

Choose a reason for hiding this comment

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

see comments above

Co-authored-by: Yaroslav Halchenko <[email protected]>
@satra satra force-pushed the doc/help-options branch from 170bbec to f3f1767 Compare July 20, 2021 02:23
@yarikoptic
Copy link
Member

so we have two failing tests in dandi-cli

  1. expected due to newer schemaversion not accepted by the api
test_publish_and_manipulate
_________________________ test_publish_and_manipulate __________________________

local_dandi_api = {'api_key': 'XXX', 'client': <dandi.dandiapi.DandiAPIClient object at 0x7fd3f60f3...di_instance(gui=None, redirector=None, api='http://localhost:8000/api'), 'instance_id': 'dandi-api-local-docker-tests'}
monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7fd3f60f3c40>
tmp_path = PosixPath('/tmp/pytest-of-runner/pytest-0/test_publish_and_manipulate0')

    def test_publish_and_manipulate(local_dandi_api, monkeypatch, tmp_path):
        client = local_dandi_api["client"]
        d = client.create_dandiset(
            "Test Dandiset",
            {
                "schemaKey": "Dandiset",
                "name": "Text Dandiset",
                "description": "A test text Dandiset",
                "contributor": [
                    {
                        "schemaKey": "Person",
                        "name": "Wodder, John",
                        "roleName": ["dcite:Author", "dcite:ContactPerson"],
                    }
                ],
                "license": ["spdx:CC0-1.0"],
                "manifestLocation": ["https://github.com/dandi/dandi-cli"],
            },
        )
        dandiset_id = d.identifier
        upload_dir = tmp_path / "upload"
        upload_dir.mkdir()
        (upload_dir / dandiset_metadata_file).write_text(f"identifier: '{dandiset_id}'\n")
        (upload_dir / "subdir").mkdir()
        (upload_dir / "subdir" / "file.txt").write_text("This is test text.\n")
        monkeypatch.chdir(upload_dir)
        monkeypatch.setenv("DANDI_API_KEY", local_dandi_api["api_key"])
        upload(
            paths=[],
            dandi_instance=local_dandi_api["instance_id"],
            devel_debug=True,
            allow_any_path=True,
            validation="skip",
        )
    
>       d.wait_until_valid()

/opt/hostedtoolcache/Python/3.8.11/x64/lib/python3.8/site-packages/dandi/tests/test_dandiapi.py:67: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = RemoteDandiset(client=<dandi.dandiapi.DandiAPIClient object at 0x7fd3f60f3f10>, identifier='000002', created=datetime....nfo=datetime.timezone.utc), modified=datetime.datetime(2021, 7, 20, 18, 56, 42, 880758, tzinfo=datetime.timezone.utc)))
min_time = 20

    def wait_until_valid(self, min_time=20):
        """
        Wait for a Dandiset to be valid.  Validation is a background celery
        task which runs asynchronously, so we need to wait for it to complete.
        """
        lgr.debug("Waiting for Dandiset %s to complete validation ...", self.identifier)
        start = time()
        while time() - start < min_time:
            r = self.client.get(f"{self.version_api_path}info/")
            if "status" not in r:
                # Running against older version of dandi-api that doesn't
                # validate
                return
            if r["status"] == "Valid":
                return
            sleep(0.5)
>       raise ValueError(
            f"Dandiset {self.identifier} is {r['status']}: {r['validation_error']}"
        )
E       ValueError: Dandiset 000002 is Invalid: Metadata version 0.5.0 is not allowed. Allowed are: 0.4.4.

/opt/hostedtoolcache/Python/3.8.11/x64/lib/python3.8/site-packages/dandi/dandiapi.py:448: ValueError
  1. "not sure" -- 400 upon publish. Most likely the same cause I guess.
test_download_newest_version upon `/publish/`
_________________________ test_download_newest_version _________________________

local_dandi_api = {'api_key': 'XXXX', 'client': <dandi.dandiapi.DandiAPIClient object at 0x7fd3f60f3...di_instance(gui=None, redirector=None, api='http://localhost:8000/api'), 'instance_id': 'dandi-api-local-docker-tests'}
text_dandiset = {'client': <dandi.dandiapi.DandiAPIClient object at 0x7fd3f60f3f10>, 'dandiset': RemoteDandiset(client=<dandi.dandiapi...e.timezone.utc))), 'dandiset_id': '000029', 'dspath': PosixPath('/tmp/pytest-of-runner/pytest-0/text_dandiset24'), ...}
tmp_path = PosixPath('/tmp/pytest-of-runner/pytest-0/test_download_newest_version0')

    def test_download_newest_version(local_dandi_api, text_dandiset, tmp_path):
        dandiset_id = text_dandiset["dandiset_id"]
        download(f"{local_dandi_api['instance'].api}/dandisets/{dandiset_id}", tmp_path)
        assert (tmp_path / dandiset_id / "file.txt").read_text() == "This is test text.\n"
>       text_dandiset["dandiset"].publish()

/opt/hostedtoolcache/Python/3.8.11/x64/lib/python3.8/site-packages/dandi/tests/test_download.py:134: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/opt/hostedtoolcache/Python/3.8.11/x64/lib/python3.8/site-packages/dandi/dandiapi.py:461: in publish
    self.client.post(f"{self.version_api_path}publish/")
/opt/hostedtoolcache/Python/3.8.11/x64/lib/python3.8/site-packages/dandi/dandiapi.py:184: in post
    return self.request("POST", path, **kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <dandi.dandiapi.DandiAPIClient object at 0x7fd3f60f3f10>, method = 'POST'
path = '/dandisets/000029/versions/draft/publish/', params = None, data = None
files = None, json = None, headers = {'accept': 'application/json'}
json_resp = True, retry = None, kwargs = {}
f = <bound method Session.post of <requests.sessions.Session object at 0x7fd3f60f3eb0>>
url = 'http://localhost:8000/api/dandisets/000029/versions/draft/publish/'
doretry = <tenacity.retry.retry_any object at 0x7fd3f479f610>
result = <Response [400]>
msg = 'Error 400 while sending POST request to http://localhost:8000/api/dandisets/000029/versions/draft/publish/'

    def request(
        self,
        method,
        path,
        params=None,
        data=None,
        files=None,
        json=None,
        headers=None,
        json_resp=True,
        retry=None,
        **kwargs,
    ):
        """
        This method looks up the appropriate method, constructs a request URL
        from the base URL, path, and parameters, and then sends the request. If
        the method is unknown or if the path is not found, an exception is
        raised, otherwise a JSON object is returned with the response.
    
        This is a convenience method to use when making basic requests that do
        not involve multipart file data that might need to be specially encoded
        or handled differently.
    
        :param method: The HTTP method to use in the request (GET, POST, etc.)
        :type method: str
        :param path: A string containing the path elements for this request.
            Note that the path string should not begin or end with the path  separator, '/'.
        :type path: str
        :param params: A dictionary mapping strings to strings, to be used
            as the key/value pairs in the request parameters.
        :type params: dict
                json=json,
                headers=headers,
                **kwargs,
            )
        except Exception:
            lgr.exception("HTTP connection failed")
            raise
    
        lgr.debug("Response: %d", result.status_code)
    
        # If success, return the json object. Otherwise throw an exception.
        if not result.ok:
            msg = f"Error {result.status_code} while sending {method} request to {url}"
            if result.status_code == 409:
                # Blob exists on server; log at DEBUG level
                lgr.debug("%s: %s", msg, result.text)
            else:
                lgr.error("%s: %s", msg, result.text)
>           raise requests.HTTPError(msg, response=result)
E           requests.exceptions.HTTPError: Error 400 while sending POST request to http://localhost:8000/api/dandisets/000029/versions/draft/publish/

what I am thinking -- may be for the purpose of the tests here dandi-api instance "supported schema version" should be set to the one of dandischema? that would help to identify moments when dandischema is about to cause a visible breakage to dandi-api. WDYT @satra?

@satra satra added minor Increment the minor version when merged release Create a release when this pr is merged labels Jul 21, 2021
@satra satra mentioned this pull request Jul 21, 2021
Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

some comments/summary from discussion

* upstream/master:
  RF(CI): run dandi-cli tests only against 3.8 (but all OSes)
  Docker image needs git to build
  Run dandi-cli tests with dandi-api image built with local version of dandischema
@satra satra force-pushed the doc/help-options branch from 5690fa5 to d95766d Compare July 27, 2021 00:48
@satra
Copy link
Member Author

satra commented Jul 27, 2021

@yarikoptic - this should be ok now even for the gui. take a look at the last commit and the overall merge. we need to remove the patch/release from PR #65

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Left some suggestions and primarily a question about schema_extra -- some documentation + comments and ideally some kind of a test seems would be very benefitial.

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

just nit picking on descriptions - should we unify to avoid the "subject of the matter" or at least harmonize the "subject" in *Asset's?

satra and others added 2 commits July 27, 2021 19:37
Co-authored-by: Yaroslav Halchenko <[email protected]>
Co-authored-by: Yaroslav Halchenko <[email protected]>
@satra satra closed this Jul 27, 2021
@satra satra reopened this Jul 27, 2021
@satra
Copy link
Member Author

satra commented Jul 28, 2021

looks like all tests are passing. the two failing checks are on released cli which does not include the docker-compose fix merged today.

@yarikoptic
Copy link
Member

eh, I hoped to release dandi-cli with dandi/dandi-cli#743 but testing just got stuck there -- never saw such to happen. but i guess we should just delay release until we also boost for new dandischema as soon that is what dandi-api starts to use.
Anyways -- I guess click Merge if satisfied ;-)

@satra satra merged commit 8201cd7 into master Jul 28, 2021
@satra satra deleted the doc/help-options branch July 28, 2021 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged release Create a release when this pr is merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants