feat (cli): initialize the CLI of GraphAr and support baseETL functions #616

jasinliu · 2024-09-03T12:17:43Z

As title.

…to graphar-cli

cli/README.md

acezen · 2024-10-27T02:39:58Z

cli/import.full.json

@@ -0,0 +1,223 @@
+{


can we put these import configuration files to testing repo? I think the configuration files are better to put together with the data.

Sure. I will move it.

cli/src/graphar_cli/graphar_cli.py

cli/src/main.cc

acezen

you can refer to 6a13df3 to fix the ORC support problem in CI.

acezen · 2024-10-27T05:26:18Z

cli/src/util.h

+
+#pragma once
+
+#include <arrow/adapters/orc/adapter.h>


acezen · 2024-10-27T05:26:51Z

cli/src/util.h

+#include <graphar/graph_info.h>
+#include <parquet/arrow/reader.h>
+
+#include <iostream>


the std library header should put at the begin.

SemyonSinchenko

Thanks a loot! It looks like a very big step forward for GraphAr. The main question in my mind at the moment is why are we going to re-define basic structures as pyantic models instead of using the code, generated from the protobuf messages? In my understanding the whole idea of proto was to avoid multiple definitions of the same things across the project.

cli/pyproject.toml

cli/src/graphar_cli/config.py

SemyonSinchenko · 2024-10-27T09:43:57Z

cli/src/graphar_cli/config.py

+
+logger = getLogger("graphar_cli")
+
+# TODO: convert to constants


I would expect to see these variables in UPPER_CASE

cli/src/graphar_cli/config.py

SemyonSinchenko · 2024-10-27T09:49:25Z

cli/src/graphar_cli/logging.py

+# under the License.
+
+import logging
+from typing import Union


I thing it should be moved to the TYPE_CHECKING block.

SemyonSinchenko · 2024-10-27T09:50:11Z

cli/src/graphar_cli/importer.py

+
+from logging import getLogger
+
+from .config import ImportConfig


I thing it can be moved to the TYPE_CHECKING block, isn't? I do not see runtime calls to this class

Can you provide some examples of using TYPE_CHECKING? It would help me a lot, thanks!

https://docs.python.org/3/library/typing.html#typing.TYPE_CHECKING

https://docs.python.org/3/library/typing.html#typing.TYPE_CHECKING

Thanks! I will try it with the protobuf changes.

cli/src/main.cc

.github/workflows/cli.yml

cli/README.md

Thespica · 2024-10-30T03:01:43Z

.github/workflows/cli.yml

+  # macos:
+  #   name: ${{ matrix.architecture }} macOS ${{ matrix.macos-version }} CLI
+  #   runs-on: macos-${{ matrix.macos-version }}
+  #   if: ${{ !contains(github.event.pull_request.title, 'WIP') && github.event.pull_request.draft == false }}
+  #   strategy:
+  #     fail-fast: false
+  #     matrix:
+  #       include:
+  #         - architecture: AMD64
+  #           macos-version: "12"
+  #         - architecture: ARM64
+  #           macos-version: "14"
+  #   steps:
+  #   - uses: actions/checkout@v3
+  #     with:
+  #         submodules: true
+
+  #   - name: Install dependencies
+  #     run: |
+  #       brew bundle --file=cpp/Brewfile
+
+
+  #   - name: Build GraphAr And Run Tests
+  #     working-directory: "cli"
+  #     run: |
+  #       pip install ./
+  #       graphar --help
+  #       graphar check -p ../testing/neo4j/MovieGraph.graph.yml
+  #       graphar show -p ../testing/neo4j/MovieGraph.graph.yml -v Person
+  #       graphar show -p ../testing/neo4j/MovieGraph.graph.yml -es Person -e ACTED_IN -ed Movie
+  #       graphar import -c import.full.yml


We can disable the job with if: false, which is considered better to me, rather than ignore the code.

similar to example in this doc:

Ps. there should also be a note to explain with why we skip this job.

Great suggestion! Thanks very much!

Thespica · 2024-10-30T12:45:30Z

.github/workflows/cli.yml

+  macos:
+    name: ${{ matrix.architecture }} macOS ${{ matrix.macos-version }} CLI
+    runs-on: macos-${{ matrix.macos-version }}
+    if: false


Can you add a note to explain why we skip this job?

maybe add a TODO

Thespica

LGTM~
Thank you!

acezen · 2024-10-30T15:10:52Z

Thanks a loot! It looks like a very big step forward for GraphAr. The main question in my mind at the moment is why are we going to re-define basic structures as pyantic models instead of using the code, generated from the protobuf messages? In my understanding the whole idea of proto was to avoid multiple definitions of the same things across the project.

Hi, Sam, The CI currently is based on the C++ library and the protobuf now not full integrate into c++ library yet, so this PR has a little re-define of the base structure but will update when the protobuf is ready.

acezen · 2024-10-30T15:57:41Z

cli/import.full.yml

@@ -0,0 +1,138 @@
+graphar:                                              # Global configuration of imported data


the import congfig files should move to testing repo too and it's better to use.the yaml as the only format to specify configurations.

acezen

the import congfig files should move to testing repo too and it's better to use.the yaml as the only format to specify configurations.

SemyonSinchenko · 2024-10-30T19:22:26Z

cli/src/graphar_cli/config.py

+    name: str
+    vertex_chunk_size: Optional[int] = 100
+    edge_chunk_size: Optional[int] = 1024
+    file_type: Literal["parquet", "orc", "csv", "json"] = DEFAULT_FILE_TYPE


I'm just curious what was the reason to use strings instead of creating a enumeration? I the file_type in at least four places and it may become challenging to support it in the future... What do you think about something like this:

from enum import Enum class FileType(str, Enum): PARQUET = "parquet" ORC = "orc" CSV = "csv" JSON = "json"

and use it instead of the string literals?

That also allows to avoid checking it each time like this if file_type not in SUPPORT_FILE_TYPES:

FileType(str, Enum) with recent versions of pydantic will raise a warning. I think it is necessary to pin a minor version.

pydantic/pydantic#9270

SemyonSinchenko

LGTM overall, I left a minor comment

…ns (apache#616) * init cli * fix include * add ci * add vertex info * change config * finish * license * fix license * remove conda recipe * enbale ci * fix typo * fix dependency * update ci * add data * fix ci * fix cmake * add arrow in cmake * add dependency * fix review * fix ci * fix ci * fix ci * fix ci * Update cli.yml * Update cli.yml * fix config type * fix ci with new testing * use enum * use enum * pin pydantic version

init cli

25ea334

jasinliu closed this Sep 3, 2024

jasinliu reopened this Sep 3, 2024

jasinliu marked this pull request as draft September 3, 2024 12:19

jasinliu added 9 commits September 5, 2024 15:13

fix include

a8da3ce

Merge branch 'main' of https://github.com/apache/incubator-graphar in…

c17de46

…to graphar-cli

add ci

54ad924

Merge branch 'main' of https://github.com/apache/incubator-graphar in…

775e559

…to graphar-cli

add vertex info

2b07a69

change config

dd72ad8

finish

5f7ed3c

Merge branch 'main' of https://github.com/apache/incubator-graphar in…

f60bd49

…to graphar-cli

license

45ab64d

jasinliu changed the title ~~[WIP] GraphAr Cli~~ GraphAr Cli Oct 25, 2024

jasinliu added 2 commits October 25, 2024 17:34

fix license

3393611

remove conda recipe

305cca5

jasinliu marked this pull request as ready for review October 25, 2024 09:37

jasinliu requested review from acezen and Thespica October 25, 2024 09:37

jasinliu added 9 commits October 25, 2024 18:00

enbale ci

cda2487

fix typo

0dc6ff4

fix dependency

8e942be

update ci

fb684fc

add data

5125d9c

fix ci

9e35d48

fix cmake

63e77ee

add arrow in cmake

3b7758a

add dependency

4d72237

acezen reviewed Oct 27, 2024

View reviewed changes

cli/README.md Outdated Show resolved Hide resolved

acezen reviewed Oct 27, 2024

View reviewed changes

cli/src/graphar_cli/graphar_cli.py Outdated Show resolved Hide resolved

acezen reviewed Oct 27, 2024

View reviewed changes

cli/src/main.cc Outdated Show resolved Hide resolved

acezen reviewed Oct 27, 2024

View reviewed changes

SemyonSinchenko reviewed Oct 27, 2024

View reviewed changes

Thespica reviewed Oct 29, 2024

View reviewed changes

cli/src/main.cc Show resolved Hide resolved

cli/src/main.cc Show resolved Hide resolved

cli/src/main.cc Show resolved Hide resolved

.github/workflows/cli.yml Outdated Show resolved Hide resolved

cli/README.md Show resolved Hide resolved

jasinliu added 8 commits October 29, 2024 15:32

fix review

9fc7353

Merge branch 'apache:main' into graphar-cli

2bee832

fix ci

77bc97c

fix ci

56aa77b

fix ci

955d609

fix ci

726ea84

Merge branch 'apache:main' into graphar-cli

6c39258

Update cli.yml

f0fe5e9

Thespica reviewed Oct 30, 2024

View reviewed changes

Update cli.yml

18ba462

Thespica reviewed Oct 30, 2024

View reviewed changes

Thespica approved these changes Oct 30, 2024

View reviewed changes

acezen approved these changes Oct 30, 2024

View reviewed changes

acezen reviewed Oct 30, 2024

View reviewed changes

acezen changed the title ~~GraphAr Cli~~ feat (cli): initialize the CLI of GraphAr and support baseETL functions Oct 30, 2024

fix config type

4af9117

SemyonSinchenko reviewed Oct 30, 2024

View reviewed changes

SemyonSinchenko approved these changes Oct 30, 2024

View reviewed changes

jasinliu added 4 commits October 31, 2024 15:00

fix ci with new testing

7b81e42

use enum

6ddac61

use enum

9f3258b

pin pydantic version

2aa64ad

acezen merged commit dcef976 into apache:main Oct 31, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat (cli): initialize the CLI of GraphAr and support baseETL functions #616

feat (cli): initialize the CLI of GraphAr and support baseETL functions #616

jasinliu commented Sep 3, 2024 •

edited

Loading

acezen Oct 27, 2024

jasinliu Oct 29, 2024

acezen left a comment

acezen Oct 27, 2024

acezen Oct 27, 2024

SemyonSinchenko left a comment

SemyonSinchenko Oct 27, 2024

SemyonSinchenko Oct 27, 2024

SemyonSinchenko Oct 27, 2024

jasinliu Oct 29, 2024

SemyonSinchenko Oct 29, 2024

jasinliu Oct 29, 2024

Thespica Oct 30, 2024 •

edited

Loading

jasinliu Oct 30, 2024

Thespica Oct 30, 2024

acezen Oct 30, 2024

Thespica left a comment

acezen commented Oct 30, 2024

acezen Oct 30, 2024

acezen left a comment

SemyonSinchenko Oct 30, 2024 •

edited

Loading

jasinliu Oct 31, 2024

jasinliu Oct 31, 2024

SemyonSinchenko left a comment


		logger = getLogger("graphar_cli")

		# TODO: convert to constants


		from logging import getLogger

		from .config import ImportConfig

		@@ -0,0 +1,138 @@
		graphar: # Global configuration of imported data


		#pragma once

		#include <arrow/adapters/orc/adapter.h>

feat (cli): initialize the CLI of GraphAr and support baseETL functions #616

feat (cli): initialize the CLI of GraphAr and support baseETL functions #616

Conversation

jasinliu commented Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acezen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SemyonSinchenko 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

Thespica Oct 30, 2024 • edited Loading

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

Thespica left a comment

Choose a reason for hiding this comment

acezen commented Oct 30, 2024

Choose a reason for hiding this comment

acezen left a comment

Choose a reason for hiding this comment

SemyonSinchenko Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SemyonSinchenko left a comment

Choose a reason for hiding this comment

jasinliu commented Sep 3, 2024 •

edited

Loading

Thespica Oct 30, 2024 •

edited

Loading

SemyonSinchenko Oct 30, 2024 •

edited

Loading