add method to upload sources from schema and metadata #82

Alig1493 · 2024-04-04T07:33:08Z

No description provided.

… sources payload

jjdelc · 2024-04-18T23:50:31Z

src/pycrunch/importing.py

@@ -52,6 +52,25 @@ def wait_for_batch_status(self, batch, status):
        else:
            raise ValueError("The batch did not reach the '%s' state in the "
                             "given time. Please check again later." % status)
+
+    def add_schema_metadata(self, ds, source_url, filename, fp, mimetype, schema, metadata):


You don't need the dataset here, it's only being used for the session, this could be obtained from the connection I believe.
The source_url is not a user input either, you will get this from the main API catalog, under catalogs.sources.

This signature should be:

Suggested change

def add_schema_metadata(self, ds, source_url, filename, fp, mimetype, schema, metadata):

def add_schema_metadata(self, schema, metadata, filename, fp, mimetype):

I think that for this case, since it's going to be crunch lake, the mimetype is always known because it has to be a parquet file, we don't accept anything else do we?

Note that schema and metadata are first, because they are the main thing about the name of this function.

Additionally, add a docstring indicating what are the types of the arguments. Particularly, schema and metadata, are they dictionaries? Or are they file pointers to my local json files?

jjdelc · 2024-04-18T23:50:39Z

tests/test_lake.py

+class TestCrunchLakeWorkflow:
+    def test_ping(self):
+        command = ["cr.core.launch", "ping", "crunch-lake-ping", '--kwargs={"pong": "test-ping"}']
+        p = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE)


What is this test?

this is to check if we have connection to the lake service from pycrunch

So it's just a temporary test? It doesn't feel like this is the right place.

@Alig1493 something to notice is that this pycrunch is an open source consumer library. It does not communicate with our internals at Crunch.

The tests we implement here should be able to run stand alone alone. We can do the integration tests in other internal repositories.

…ing. move test_lake from pycrunch tests to functests in zoom

jjdelc

You will need a test for this, read through the pycrunch tests to see how they are built. Note that they are all offline tests with stubs

jjdelc · 2024-04-25T12:18:06Z

src/pycrunch/elements.py

@@ -283,6 +283,7 @@ class ElementResponseHandler(lemonpy.ResponseHandler):
    parsers = {"application/json": parse_json_element_from_response}

    def status_401(self, r):
+        print(r.json())


Remove this print

jjdelc · 2024-04-25T12:22:27Z

src/pycrunch/importing.py

+            }
+        )
+        return shoji.Entity(site.session, body={
+                "status_code": response.status_code,


These aren't the attributes of a new Source entity.

These body attributes should match the same as if you were reading a Source from the API. At this point right after the creation you won't have them. The response will probably be a 201 with a Location header.

So, the only attribute you should be able to fill form there is the self.

See how it's being done here:
https://github.com/Crunch-io/pycrunch/blob/master/src/pycrunch/shoji.py#L180

entity["self"] = URL(seeother[-1].headers["Location"], "")

The Entity you return is ok to be empty as long as it has a self url.

Then the consumer will be able to do source.refresh() to trigger a new GET request and fetch all the attributes if they want.

jjdelc · 2024-04-25T12:22:59Z

src/pycrunch/importing.py

+                "schema": schema, 
+                "metadata": metadata,
+                "crunchlake": "create",
+                "dataset_id": "None"


There is no dataset ID to create a new source. Sources are not linked to datasets.

The argument is required for the sources post request endpoint. Otherwise there's errors.
See here: https://github.com/Crunch-io/zoom/blob/master/server/tests/controllers/test_sources.py#L282

Then it looks like that is incorrect. Sources are not linked to datasets.

This controller should not create sources depending on datasets. The relationship is the other way around. Sources do not point to datasets. Datasets point to sources.

I'm simply making a post request with the help of pycrunch to the endpoint that requires a dataset_id to be added. Otherwise I get the following error:

E pycrunch.lemonpy.ServerError: (<Response [500]>, 'https://test01.crunch.io/api/sources/', {'status': 500, 'exception': ['cr.lib.files.CrunchLakeMissingDatasetID: attr dataset_id: str is required for purposes of CrunchLake\n'], 'traceback': [['/var/lib/crunch.io/crserver_api_venv_python3.6/lib/python3.6/site-packages/cherrypy/_cprequest.py', 670, 'respond', 'response.body = self.handler()'], ['/var/lib/crunch.io/crserver_api_venv_python3.6/lib/python3.6/site-packages/cherrypy/lib/encoding.py', 221, '__call__', 'self.body = self.oldhandler(*args, **kwargs)'], ['/var/lib/crunch.io/crserver_api_venv_python3.6/lib/python3.6/site-packages/cr/core/monitoring/tracer.py', 903, 'inner', 'return f(*args, **kwargs)'], ['/var/lib/crunch.io/crserver_api_venv_python3.6/lib/python3.6/site-packages/cr/server/api/controllers/tools.py', 705, 'tracing_inner', 'return oldhandler(*args, **kwargs)'], ['/var/lib/crunch.io/crserver_api_venv_python3.6/lib/python3.6/site-packages/cr/server/shoji.py', 162, 'shojify_inner', 'result = request._pre_shojify_handler(*args, **kwargs)'], ['/var/lib/crunch.io/crserver_api_venv_python3.6/lib/python3.6/site-packages/cherrypy/_cpdispatch.py', 60, '__call__', 'return self.callable(*self.args, **self.kwargs)'], ['/var/lib/crunch.io/crserver_api_venv_python3.6/lib/python3.6/site-packages/cr/server/api/controllers/sources.py', 161, 'POST', '**options,'], ['/var/lib/crunch.io/crserver_api_venv_python3.6/lib/python3.6/site-packages/cr/core/monitoring/tracer.py', 903, 'inner', 'return f(*args, **kwargs)'], ['/var/lib/crunch.io/crserver_api_venv_python3.6/lib/python3.6/site-packages/cr/lib/entities/sources.py', 1049, 'from_file', 'creation_time=data["creation_time"],'], ['/var/lib/crunch.io/crserver_api_venv_python3.6/lib/python3.6/site-packages/cr/lib/files/__init__.py', 215, '__init__', '"attr dataset_id: str is required for purposes of CrunchLake"']], 'message': "['cr.lib.files.CrunchLakeMissingDatasetID: attr dataset_id: str is required for purposes of CrunchLake\\n']"})

Yes, I think this is a bug that has been introduced on the backend. And we should not grow this bug. Sources do not hold references to Datasets. Because multiple datasets are allowed to source rows from the same source entity. In which case, which of those multiple datasets does Source.dataset_id is pointing to?

@mgdotdev Why does the CrunchLake source require to know about a Dataset ID? For a fresh user, they will first have to upload a parquet file while they do not have any Datasets existing in the system yet. And only later they can create a Dataset by using this source. This relation is backwards.

jjdelc · 2024-04-25T12:23:27Z

src/pycrunch/importing.py

+            data={
+                "schema": schema, 
+                "metadata": metadata,
+                "crunchlake": "create",


What is this argument?

It's crunchlake specific. @mgdotdev can expand on this.

jjdelc · 2024-04-25T19:14:53Z

src/pycrunch/importing.py

+
+        Parameters:
+        site (shoji.Catalog): a shoji Catalog object, from which we acquire session and sources url
+        schema (str): json string containing schema


Do not require the user to provide a JSON string. Make it accept dictionaries. If this needs to convert to JSON we do it internally.

jjdelc · 2024-04-25T19:14:56Z

src/pycrunch/importing.py

+        Parameters:
+        site (shoji.Catalog): a shoji Catalog object, from which we acquire session and sources url
+        schema (str): json string containing schema
+        metadata (str): json string containing metadata


jjdelc · 2024-04-25T19:18:59Z

src/pycrunch/importing.py

+                "schema": schema, 
+                "metadata": metadata,
+                "crunchlake": "create",
+                "dataset_id": "None"


Then it looks like that is incorrect. Sources are not linked to datasets.

This controller should not create sources depending on datasets. The relationship is the other way around. Sources do not point to datasets. Datasets point to sources.

joaquimadraz · 2024-05-23T10:24:20Z

@jjdelc @Alig1493 I think this is the wrong abstraction. In a nutshell, we only need to add the option to send extra data when adding a source. Here's a PR #83

joaquimadraz · 2024-05-24T15:31:57Z

Closed in favour of #83

add ping test for crunchlake consumers

a4ea5d0

Alig1493 self-assigned this Apr 4, 2024

add source post with schema and metadata params. add debug method for…

77e2c0d

… sources payload

jjdelc reviewed Apr 24, 2024

View reviewed changes

Alig1493 added 2 commits April 24, 2024 12:03

finalize payload and response for sources, schema and metadata upload

e550231

ammend add_schema_metadata to only accept required params. add docstr…

8d9e924

…ing. move test_lake from pycrunch tests to functests in zoom

Alig1493 changed the title ~~add ping test for crunchlake consumers~~ add method to upload sources from schema and metadata Apr 25, 2024

jjdelc requested changes Apr 25, 2024

View reviewed changes

remove stray print. returning result as shoji.entity

f7d0546

Alig1493 force-pushed the crunchhlake-consumer-integration-186820887 branch from a137d4c to f7d0546 Compare April 25, 2024 14:04

jjdelc requested changes Apr 25, 2024

View reviewed changes

add json formatting

01f2a2d

joaquimadraz closed this May 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add method to upload sources from schema and metadata #82

add method to upload sources from schema and metadata #82

Alig1493 commented Apr 4, 2024

jjdelc Apr 18, 2024

jjdelc Apr 18, 2024

Alig1493 Apr 24, 2024

joaquimadraz Apr 24, 2024

jjdelc Apr 25, 2024

jjdelc left a comment

jjdelc Apr 25, 2024

jjdelc Apr 25, 2024

jjdelc Apr 25, 2024

Alig1493 Apr 25, 2024

jjdelc Apr 25, 2024

Alig1493 Apr 25, 2024

jjdelc Apr 26, 2024

jjdelc Apr 25, 2024

Alig1493 Apr 25, 2024

jjdelc Apr 25, 2024

jjdelc Apr 25, 2024

jjdelc Apr 25, 2024

joaquimadraz commented May 23, 2024

joaquimadraz commented May 24, 2024

	def add_schema_metadata(self, ds, source_url, filename, fp, mimetype, schema, metadata):
	def add_schema_metadata(self, schema, metadata, filename, fp, mimetype):

add method to upload sources from schema and metadata #82

add method to upload sources from schema and metadata #82

Conversation

Alig1493 commented Apr 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjdelc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaquimadraz commented May 23, 2024

joaquimadraz commented May 24, 2024