Initial version of JanusGraph Python client drivers #4

debasishdebs · 2018-09-04T13:15:02Z

Fixes #1

This is 0.9.0 version.

TODO:
Move to 1.0.0 & create 1.1.0 versions corresponding to JanusGraph 0.3.0 and Master.
Add docs for "Contributions to Lib"
Add docs "How to create connection"
Update Publish Repo from Test PyPi to PyPi

Signed-off-by: Debasish Kanhar [email protected]

sjudeng

This looks really good! Looks well documented both at the user guide and API level. Also good to see the unit testing. Nice work!

I took a quick pass through the code and added some initial comments below. I can go through integration testing and documentation review later as well, though independent reviews from anyone else in the community would be welcome as well.

sjudeng · 2018-09-04T23:38:19Z

.gitignore

+
+# mypy
+.mypy_cache/
+


Can you trim the .gitignore file down to just what's necessary for this repo? Some sections don't seem to apply here.

Was this done? Looks like the current contents are very much Python-related... If so, please resolve this thread.

.travis.yml

build.py

src/main/scripts/janusgraph-python

src/unittest/python/__init__.py

src/unittest/python/serialization/GeoShapeDeserializer_test.py

src/unittest/python/core/datatypes/Circle_test.py

.travis.yml

FlorianHockmann

I just made a quick first pass through.

Really nice to see that you already included API docs! Could you make a preview of those docs available? Maybe in a dedicated gh-pages branch via GitHub?

README.md

build.py

README.md

debasishdebs · 2018-09-06T18:29:18Z

@FlorianHockmann : I was having problems hosting with Github Pages. I've hosted under readthedocs .
Currently, I have't added links from index page to API docs, and the index currently contain only User Guide docs. We can add a ToC kind of thing for API docs. What would be your suggestion on how to add links to API docs.

mbrukman

Please also remove the dist/janusgraph_python-0.0.9.tar.gz from this PR, it shouldn't be submitted, and this file should be .gitignored if it isn't already.

.travis.yml

LICENSE

mbrukman · 2018-09-07T02:07:26Z

README.md

+
+For installing PyBuilder, the following needs to be run:
+
+```pip install pybuilder```


Please use 3 lines for this code sample: the tree backticks on the first line, the command on the next line, and the 3 backticks again.

Keeping them on a single line doesn't get a full-width text box (that looks like a terminal), but a locally-formatted line, no different than a single backtick, and doesn't look as nice.

Is there a minimum version of pybuilder expected or is it generally expected to work with the latest version?

@pluradj : Nopes, there is no minimum version. It has been tesed against latest, and is expected to work against latest.

mbrukman · 2018-09-07T02:07:33Z

README.md

+
+Once PyBuilder is installed, run the following commands to build the Library:
+
+```pyb```


Same here as above.

mbrukman · 2018-09-07T02:14:19Z

src/main/__init__.py

@@ -0,0 +1,22 @@
+"""


Please use # comments for license headers in Python, as mentioned elsewhere.

mbrukman · 2018-09-07T02:14:24Z

src/main/python/janusgraph_python/__init__.py

@@ -0,0 +1,22 @@
+"""


Please use # comments for license headers in Python, as mentioned elsewhere.

mbrukman · 2018-09-07T02:16:22Z

src/main/python/janusgraph_python/core/attribute/GeoPredicate/GeoContains.py

+limitations under the License.
+"""
+
+__author__ = "Debasish Kanhar (https://github.com/debasishdebs)"


Aren't we saying that this project is "copyright JanusGraph Python Authors"?

Will each file have a separate author line? What if someone rewrites this file entirely? Are they still a contributor, or do they replace the author? Can you only have a single author?

Basically, I'm asking whether we need to have author, credits, email, etc. for each and every file, or if we can just keep an AUTHORS.txt and CONTRIBUTORS.txt for this repo, similarly as is done in the main JanusGraph repo, and maintain it there centrally, rather than in every file individually.

Thoughts?

@mbrukman : I would prefer keeping just the author and credits in each file in addition to having AUTHORS.txt. That way we can have who was primary developer for a particular file while having AUTHORS.txt won't help us in pinpointing the same.

Also, another reason why I'm for having those is that, in future if someone wants to extend the library, they will know the exact person to contact to depending on author rather than contacting everyone on AUTHORS.txt.

I deleted all other fields like email, version etc but kept just author and credits. As for your question is author a single value element, well no, we can keep adding more and more authors as comma separated in same variable

Your thoughts?

@debasishdebs I think all of the __ fields should be removed. With this PR, you are contributing the code to JanusGraph which is a community-supported project. You are the author of these files, a fact and credit that will never change, and it will be reflected in the GitHub history if someone wants to find out who is the original author.

The best avenue for support is to leverage the public channels (mailing list, issues tracker, etc) so the entire community can continue to support the code, for example if you are too busy or if you are no longer involved with the project. Let's say 5 years from now, the JanusGraph Python driver is wildly popular, but you no longer work on it. You might not want or be able to help developers asking you questions directly about code you developed 5 years ago.

Got it, never gave a thought to 5 or 10 years down the line scenario. :-D
I've removed all those, and should be addressed in next commit.

src/main/python/janusgraph_python/core/attribute/GeoPredicate/GeoWithin.py

mbrukman · 2018-09-07T02:18:22Z

src/main/python/janusgraph_python/core/attribute/TextPredicate/Text.py

+    @staticmethod
+    def textContainsRegex(value):
+        """
+        Implements JanusGraph's textContainsPrefix functionality.


This should be on the previous line, right after the """.

See https://www.python.org/dev/peps/pep-0257/

Similarly elsewhere.

We can create an issue to track this, though both versions are minor errors, in terms of indendation, and can be ignored.

pluradj

Thanks for this contribution @debasishdebs! It is looking pretty good so far.

pluradj · 2018-09-07T15:28:23Z

docs/conf.py

+
+# General information about the project.
+project = u'JanusGraph-Python docs'
+copyright = u'2018, Debasish Kanhar'


JanusGraph Python Authors

build.py

pluradj · 2018-09-07T15:32:03Z

docs/geo-predicates.rst

+``GeoShape(India) contains GeoShape(New Delhi)`` holds true.
+
+For scope of example, we have added a point named arcadia with co-ordinates 7.58 21.50 and radius 5km (Circle).
+For reference, see the docs about `GeoShapes <geo-shapes.html>`_ (Note how latitude nad longitude are reversed).


nit s/nad/and

pluradj · 2018-09-07T15:44:26Z

README.md

@@ -6,3 +6,36 @@ JanusGraph Python driver code is provided under the [Apache 2.0
 license](APACHE-2.0.txt) and documentation is provided under the [CC-BY-4.0


actually, I'll push those fixes separately before we commit this PR.

Note the fixes I mentioned above are merged now.

pluradj · 2018-09-07T15:51:14Z

README.md

+
+
+For building the library, the pre requisite is that PyBuilder needs to be installed on system.
+


The first prerequisite should be what the minimum supported Python versions are.

pluradj · 2018-09-07T18:11:00Z

docs/text-predicates.rst

+
+This Predicate is part of *Text* package.
+TextContainsFuzzy is used to query for partial matches with Fuzzy logic in values of properties.
+The following example is based on GodsOfGraph which comes pre-packaged with JanusGraph.


nit s/GodsOfGraph/Graph of the Gods

You can mention closer to the top that all of the examples are based on Graph of the Gods rather than repeating it in the docs for each predicate.

pluradj · 2018-09-07T18:14:12Z

requirements.txt

@@ -0,0 +1 @@
+dist/janusgraph_python-0.0.9.tar.gz


is this file requirements.txt needed?

@pluradj : This was needed so that I could host a preview of docs on readthedocs . I'll me cleaning this once we are in final process for merge request.

so you'll need to remove dist/janusgraph_python-0.0.9.tar.gz before merge

Yes, once we have approavals from all commiters, I can then go ahead and remove the requirements.txt as well as the directory containing that .tar.gz file

I'm concerned that if we get sufficient approvals to merge this PR, it'll be merged with the extra files, and history is permanent, i.e., we won't be able to remove the *.tar.gz without destructively modifying/rewriting history.

You can keep these files locally, but would you mind removing them from the PR for review purposes, please?

Not an issue. This was primarily required so as to host the docs on public website for people to view it. But since a lot / those who needed to view must have already viewed it, I'm going to remove that in next commit.

pluradj · 2018-09-07T18:17:20Z

src/main/python/janusgraph_python/core/datatypes/Circle.py

+
+
+class Circle(object):
+    def __init__(self, longitude, latitude, radiusInKM):


self, latitude, longitude, radiusInKM

pluradj · 2018-09-07T18:17:53Z

src/main/python/janusgraph_python/core/datatypes/GeoShape.py

+    """
+
+    @staticmethod
+    def Point(longitude, latitude):


latitude, longitude

pluradj · 2018-09-07T18:18:34Z

src/main/python/janusgraph_python/core/datatypes/GeoShape.py

+        return point
+
+    @staticmethod
+    def Circle(longitude, latitude, radiusInKM):


latitude, longitude, radiusInKM

mbrukman · 2018-09-09T22:00:51Z

docs/build.py

+
+    project.set_property("dir_dist", "target/dist/" + project.name)
+    project.depends_on("gremlinpython", "=={}".format(tinkerpop_version))
+    pass


This is only used as a noop placeholder when the body of a function is empty; why do we need it here?

https://docs.python.org/2/tutorial/controlflow.html#pass-statements

Well @mbrukman , its just good coding practice in Python to keep pass statement when the function doesn't return anything. Surely I can remove that as that won't break anything else :-)

mbrukman · 2018-09-09T22:01:22Z

docs/build.py

+"""
+
+__author__ = "Debasish Kanhar (https://github.com/debasishdebs)"
+__credits__ = ["Jason Plurad", "sjudeng", "Florian Hockmann", "Misha Brukman"]


Very kind of you to include me here, but maybe would be best to remove the author/credits/license/email fields entirely?

Please check my comment .
I certainly can remove it if all of us reach a deciding point :-)

mbrukman · 2018-09-09T22:03:41Z

README.md

+    - [Python 3.6](https://www.python.org/download/releases/3.6.0/).
+
+Once the required `Python` version is installed on system, the following requirements needs


No need to have code formatting on this line, it's a name, not a command.

mbrukman · 2018-09-09T22:04:19Z

README.md

+
+- VirtualEnv:
+    -   ```


No need to have a bullet in front of the code block, just put it under the section and indent it, and it'll look right.

mbrukman · 2018-09-09T22:04:25Z

README.md

+        ```
+- PyBuilder:
+    - ```


Similarly here.

mbrukman · 2018-09-09T22:05:08Z

README.md

+        ```bash
+        # For Unix systems
+         bash build-docs.sh


Extra space before bash?

Also, why not make the script executable and just use ./build-docs.sh instead?

mbrukman · 2018-09-09T22:05:16Z

README.md

+        ```
+
+Once done, you can see the build HTML files under `docs/_build` directory.


s/build/built/

mbrukman · 2018-09-09T22:05:27Z

README.md

+         ```
+
+    - To install the library (After Building library):


s/After/after/

mbrukman · 2018-09-09T22:05:34Z

README.md

+
+- Building Library:
+    - To build the library run the following command:


s/library run/library, run/

Sorry, wrong click in the review box. I meant to add comments, not approve. Reverting approval.

README.md

mbrukman · 2018-09-13T07:11:03Z

.travis.yml

@@ -30,15 +30,15 @@ install:

 before_script:
  # Install all pre-requisites for building library and docs
-  - bash before_script.sh ${ENV_NAME}
+  - bash before-script.sh ${ENV_NAME}


Quotes around ${ENV_NAME}, please.

these don't appear to be fixed yet in this PR

mbrukman · 2018-09-13T07:11:06Z

.travis.yml


 script:
  # Build library and docs
-  - bash build.sh ${ENV_NAME}
+  - bash build-library.sh ${ENV_NAME}


Quotes around ${ENV_NAME}, please.

mbrukman · 2018-09-13T07:12:26Z

build-docs.sh

-ENV_NAME=$1
+if [ -z "$1" ]
+then
+      echo "Usage $0 [Env Name]"


$(basename "$0") will get you just the name of the script, without the path leading to it, which will make for cleaner output.

mbrukman · 2018-09-13T07:13:50Z

build.sh

 # Build Library first
 # Re-activate the Virtual environment
-source ${ENV_NAME}/bin/activate
+unameOut="$(uname -s)"


This var is only used in one place, why not put it directly into the case statement?

mbrukman · 2018-09-13T07:14:33Z

src/main/python/janusgraph_python/core/datatypes/Point.py

+
+
+class Point(object):
+    def __init__(self, longitude, latitude):


Looks like this was not addressed? Still reads longtitute, latitude.

docs/installation.rst

mbrukman · 2018-09-13T07:20:37Z

docs/installation.rst


-    # To build documentation,
+    # To build only documentation,


To build only the documentation, run:

mbrukman · 2018-09-13T07:23:01Z

install-library.sh

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+if [ -z "$1" ]


How about single line:

if [ -z "${1:-}" ]; then

The "${1:-}" defaults the value to the empty string if it's unspecified. This also lets you add -eu to the runline, or set -eu as the first command to enable strict mode:

-e will exit if any command had an error (exited with non-zero status code)

-u will exit if any variable is undefined

Thus, if $1 is undefined, "${1:-}" is still safe to evaluate even if set -u is enabled as it will default to the empty string.

mbrukman · 2018-09-13T07:23:20Z

install-library.sh

+
+if [ -z "$1" ]
+then
+      echo "Usage $0 [Env Name]. Defaulting to tempENV env name"


I think we have 2-space indent for shell scripts elsewhere (Python should stay with 4 spaces). Would you mind updating it to this?

mbrukman · 2018-09-13T07:23:57Z

build.sh

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+if [ -z "$1" ]


There's a lot of duplication among all the shell scripts. Any chance this can be factored out into a common shell script with functions that can be reused from other scripts?

@mbrukman : Created a new script namely build.sh and removed the redundant ones. Acccordingly updated installation docs in fresh commit.

Dynamic tinkerpop JanusGraph#4

chupman

On mac I had to build with sudo -H pyb. Might be worth it to add it under it's own section.

chupman · 2018-10-03T23:12:53Z

README.md

+
+    - [Python 3.4](https://www.python.org/download/releases/3.4.0/).
+    - [Python 3.5](https://www.python.org/download/releases/3.5.0/).
+    - [Python 3.6](https://www.python.org/download/releases/3.6.0/).


These links are dead. Looks like they were moved to a format of https://www.python.org/downloads/release/python-360/

chupman · 2018-10-03T23:19:41Z

README.md

+
+- VirtualEnv:
+    ```
+    pip install virtualenv


I just tried to build the library on osx and CentOS and in both cases it's defaulting to running python2. It's been awhile since I've worked in Python, but shouldn't the unix system scripts use pip3 and python3 instead of pip and python.

That being said I'm getting build failures on unit test errors right now when I'm trying to build the library when I do use python3. When the build script tries to use python2 it's not getting that far.

Sorry I was trying to build your 1.0-beta.1 branch. I was able to build the library on both CentOS and mac when I was pointed at the correct branch.

FlorianHockmann

I just made a first pass through the actual implementation, although I didn't manage to review everything yet.
I added a few comments, but they are basically all only about stylistic issues.

FlorianHockmann · 2018-10-05T16:58:22Z

src/main/python/janusgraph_python/core/attribute/GeoPredicate/GeoContains.py

+from janusgraph_python.core.datatypes.GeoShape import GeoShape
+
+
+class GeoContains(object):


What's the purpose of this class? Wouldn't it be simpler to just let Geo.geoContains() directly return P("geoContains", value)?
That would be a lot more readable in my opinion as it doesn't require to jump to another file for something that could be a one-liner. You also already did it like that for the Text predicates. So, why not for Geo predicates?
(The same of course applies to other Geo predicate classes.)

Actually, thanks for pointing that out. Had planned to, but missed out completely. What I'm gonna do in fresh commit is, I'm deleting the 2 packages namely attribute.TextPredicate & attribute.GeoPredicate. I've modelled Geo class the same way as Text class per your comments, and moved them under attribute package.

FlorianHockmann · 2018-10-05T17:05:19Z

src/main/python/janusgraph_python/core/attribute/TextPredicate/Text.py

+            P
+        """
+
+        predicate = P("textContains", value)


It's just a nit and I think @mbrukman already commented on this somewhere, but I would also remove these temp variables like predicate and instead directly return P("textContains", value). This increases readability in my opinion as it doesn't involve another variable that isn't really needed.
It's also a common refactoring called Inline Temp, see [1] and [2].

I've done that in fresh commit for both Text and Geo classes

FlorianHockmann · 2018-10-05T17:08:32Z

src/main/python/janusgraph_python/core/attribute/GeoPredicate/Geo.py

+            value (GeoShape): The GeoShape to query for and return all results which are present inside this GeoShape
+
+        Returns:
+            Any


Any? Shouldn't this include the return type (bool) and tell the reader when it returns true and when false?

@FlorianHockmann : I think it will not be Any, but might be of type GeoShape I guess. Because, if the query returns to True, the result of Predicate is actual Geometrical object right? Either a single of list depending on how it is used with next() or toList()

Basically the same as for geoContains: Predicates are functions that only return a boolean and Gremlin steps use these predicates to filter the objects they are currently traversing. The TinkerPop reference docs contain a more detailed description.

Ah. Thanks for the clarification. What I've done now is I've updated the return type for both Geo and Text predicate classes accordingly.
But, if that is the case, shouldn't it be same in your PR? I see you have specified return type of Text Predicates as just Text Predicate here

FlorianHockmann · 2018-10-05T17:10:42Z

src/main/python/janusgraph_python/core/attribute/GeoPredicate/Geo.py

+    def geoContains(value):
+        """ The class is used for JanusGraph geoContains predicate.
+
+        GeoContains predicate holds true when one object, is contained by another. Considering the example in docs,


Not that important and I'm not a native speaker, but I believe there shouldn't be a comma here: when one object, is contained by another
Apart from that: I don't understand the second sentence. Which example from which docs? I think these docstrings should be self-contained and not reference some other docs that the reader doesn't have available when reading this.

@FlorianHockmann : Thanks. Nice catch :+1 . Also, I've updated the statements here, and rather made it self explanatory.

FlorianHockmann · 2018-10-05T17:12:26Z

src/main/python/janusgraph_python/core/attribute/GeoPredicate/Geo.py

+        the following statement holds true : India (GeoShape) contains (GeoContains) New Delhi (GeoShape)
+
+        Args:
+            value (GeoShape): The GeoShape to query for and return all results which are present inside this GeoShape


and return all results which are present inside this GeoShape <- This doesn't sound right. A predicate just returns true or false, indicating whether the currently traversed element fulfils the predicate or not.

@FlorianHockmann : Is it?
Because per my understanding, when I do query something like g.V().has("name", textContains("something").next() from gremlin console, the expected output is, the list of vertices which contains the passed string. Its not true or false.

Might be a silly question, but will clarify my understanding a lot if you can help :-)

That is true, but it's the output of the full traversal. The predicate itself just returns true or false which is then used to filter the traversed vertices by the has step.

FlorianHockmann · 2018-10-05T17:22:34Z

src/main/python/janusgraph_python/core/datatypes/Circle.py

+
+        status = self.__are_valid_coordinates()
+
+        if not status:


(nitpick) Here, I would again remove the temp variable status. Reading if not status doesn't tell me anything. I still have to read the line above to understand this. If it would be simply if not self.__are_valid_coordinates() (or even better if self._are_coordinates_invalid to avoid the negation), then I would directly understand the if clause.

@FlorianHockmann : Haha. Nice catch. Really makes it more readable now :-)

FlorianHockmann · 2018-10-05T17:27:50Z

src/main/python/janusgraph_python/core/datatypes/Circle.py

+        """
+        return "CIRCLE(lat: {}, lon: {}, r: {})".format(self.getLatitude(), self.getLongitude(), self.getRadius())
+
+    def getLatitude(self):


Aren't getters like these extremely unusual as the attributes can be accessed directly?

Yes, but what would be your suggestion? Make them privates objects of class? Bit of nitpick cleaning is to be done, hence thanks for reviewing those set of classes to point me there :-)

Isn't the pythonic way to simply let users access the attributes directly like circle.latitude instead of doing circle.getLatitude()?

@FlorianHockmann : Well, they can be made accessible, but why should we? When a user creates a Circle object, he is essentially creating a JanusGraph GeoShape. The latitude, longitude etc are its intrinsic properties, and I feel shouldn't be made public direction. Hence I added the methods for all 3 class attributes. Rather I made the latitude, longitude, radius are private variables. Let me know if that seems a good reason to keep it this way. :-)

I still think that it's unusual in Python to use getters or private variables in cases like this. See for example how Gremlin-Python's data classes work. The Element class has an id and a label variable and doesn't use any getters. All other classes there have the same structure and I don't see why latitude or longitude should be made private when id and label are not.
But maybe we can get an opinion of a Python developer on this one?

Got that. Looks like yes the users of Element class can access the attribute directly too. Like I do for accessing Vertex ID or Edge ID.

Ah. Looks like we are on same page regarding having a base class, and multiple GeoShapes inheriting from that class, corresponding to Point, Circle etc. If we do agree on the same, should I create an issue to track the same? I can close out the issue as part of our next minor release, where I'm planning to add schema element creation to library (Which is 70-80% done)

I don't think that adding a base class really matters for this issue. I just think that adding getters for attributes is a bad practice in Python. There's really no point in having lines 40-78 that do nothing but return attributes.

I can still do that, but then I'll have to update the unit tests accodingly, and bit of code too. If we seriously want not to have methods just to retrive values, and rather use getters, then I can create an issue to track the same, as that is bit more extensive work.

The problem is that you can't just change that in a future version as removing methods is a breaking change. If you don't want to change it in this PR, then it should be done in another one before the first release in my opinion.

+1. Let keep this in mind. This is not the only PR that will go in before the first release. We don't need perfection on this PR. Let's continue opening issues for things that can be addressed in separate PRs.

FlorianHockmann · 2018-10-05T17:29:12Z

src/main/python/janusgraph_python/core/datatypes/Circle.py

+        """
+        return "CIRCLE"
+
+    def toString(self):


Isn't the usual way in Python to just override __str__? Where's the point in also having an additional toString() method?

I added that initially as a mistake, planned to remove but forgot. Thanks for pointing that out @FlorianHockmann :-)

FlorianHockmann · 2018-10-05T17:31:17Z

src/main/python/janusgraph_python/core/datatypes/Circle.py

+            bool
+        """
+
+        if type(other) is str and other == self.toString():


The docs for this method indicate that other should be a Circle. So, in which case would other be a string?

The other scenario @FlorianHockmann being well when someone wants to check for equality between actual Circle object and its String representation. I'm not able to recollect exactly why I felt the need for it because I remember initially, I didn't have the check against string type. What I'll do is, try removing them, building and check if nothing breaks.

FlorianHockmann · 2018-10-05T17:33:54Z

src/main/python/janusgraph_python/core/datatypes/GeoShape.py

+    @staticmethod
+    def Point(longitude, latitude):
+        """
+        This is wrapper method to call the actual Geographical Point class.


This comment only explains the implementation instead of telling the user what the method does. How about something like Creates a Point with the given coordinates. instead?

I've updated docs for Point based on my previous comments for Circle class. The docs and methods are analogous.

chupman · 2018-10-10T18:30:04Z

src/main/python/janusgraph_python/driver/ClientBuilder.py

+                graphson_reader = kwargs["graphson_reader"]
+                graphson_writer = kwargs["graphson_writer"]
+            else:
+                raise AttributeError("Additional parameters if provided needs to be keywords arguments of "


I setup a connection with url, port, and graph and get an error that I need to set graphson_reader and graphson_writer even though it looks like kwargs should be None.

connection = JanusGraphClient().connect(url="0.0.0.0", port="8182", graph="airroutes_traversal").get_connection() File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/site-packages/janusgraph_python/driver/ClientBuilder.py", line 74, in connect raise AttributeError("Additional parameters if provided needs to be keywords arguments of " AttributeError: Additional parameters if provided needs to be keywords arguments of `graphson_reader` and `graphson_writer`

@chupman : Are you on latest code? I remember having that bug, and I had pushed fix resolving the same on this PR. The latest code on fix#3 branch has fixed this issue. I was able to get the following thing working, without any error:

connection = JanusGraphClient().connect(url="13.71.86.xxx", port="8182", graph="g").get_connection() g = Graph().traversal().withRemote(connection) print("Count: ", g.V().count().next())

I built it last week so it should be the most current, but I'll try to build it again to validate.

I'm going to try with a fresh Ubuntu VM, but so far on my CentOS VM and my macbook I'm unable to build the library today.

if I run ./build=.sh, the command will complete, but the library is never built

Building library ./build.sh: line 65: ./build-library.sh: Permission denied

If I use pyb

[root@some-vm janusgraph-python]# pyb PyBuilder version 0.11.17 Build started at 2018-10-11 13:15:17 ------------------------------------------------------------ [INFO] Building janusgraph_python version 0.0.9 [INFO] Executing build in /root/janusgraph-python [INFO] Going to execute tasks: clean, install_dependencies, prepare, compile_sources, package, publish [INFO] Removing target directory /root/janusgraph-python/target [INFO] Installing all dependencies [INFO] Running unit tests [WARN] unittest_file_suffix is deprecated, please use unittest_module_glob [INFO] Executing unit tests from Python modules in /root/janusgraph-python/src/unittest/python [INFO] Executed 10 unit tests [ERROR] Test has error: unittest.loader._FailedTest.RelationIdentifierSerializer_test [ERROR] Test has error: unittest.loader._FailedTest.RelationIdentifierDeserializer_test [ERROR] Test has error: unittest.loader._FailedTest.CircleSerializer_test [ERROR] Test has error: unittest.loader._FailedTest.GeoShapeDeserializer_test [ERROR] Test has error: unittest.loader._FailedTest.PointSerializer_test [ERROR] Test has error: unittest.loader._FailedTest.RelationIdentifier_test [ERROR] Test has error: unittest.loader._FailedTest.Circle_test [ERROR] Test has error: unittest.loader._FailedTest.Point_test [ERROR] Test has error: unittest.loader._FailedTest.GraphsonReader_test [ERROR] Test has error: unittest.loader._FailedTest.GraphsonWriter_test ------------------------------------------------------------ BUILD FAILED - There were 10 error(s) and 0 failure(s) in unit tests ------------------------------------------------------------ Build finished at 2018-10-11 13:15:19 Build took 2 seconds (2040 ms) [root@some-vm janusgraph-python]# git status # On branch fix_#3

@chupman : I just spawned off a fresh Ubuntu VM, and I was able to make it work. I have tested it just on Windows machines till now. I'll test with Ubuntu and let me know.

As for testing on CentOS, well I plan on downloading a base CentOS docker image, pulling repo inside and testing it. This is a real strange issue, and don't know why all tests are failing on your end. Can you do pyb -v in verbose mode so that I can check the logs?

This is a real nice thing you are doing. We ideally would want to test in on all OSs possible before releasing. In meantime if you can get logs for pyb -v that will be great

@chupman : Looks like you are getting permission denied on build.sh. Did you first do chmod +x build.sh to give it exec permissions?

Somehow I suspect that the shell script in your case is picking up default Python, i.e. python2 not Pythn3. Note that the libs aren't supported on python2.

I build the library fresh from scratch on my most updated repo (debasishdebs/janusgraph-py) and it was failing initially, coz it was picking up the default python2 not 3. Reason being python 2 provides __cmp()__ to assert equality while that was changes to __eq()__ and __neq()__ in python3.

I'll have better idea wen you can do following things:

Print the whole log of ./build.sh -d true -b true -i true. I do print the python version being used each time before building docs and before building lib. That will help accertain if the error is because of Python versions.

If Python version is still 3.X, then run just pyb -v instead of pyb which is in verbose mode to have better idea why build is failing.

If its the 1st one, as per my suspicion, then I've a commit ready. Its already pushed to my personal repo (janugraph-py). If the reason is confirmed, I can push the commit to this PR and update the build scripts accordingly.

The Python version is printed somewhat in similar way as follows:

On my Mac I ran into a different issue. I'm not sure if pybuilder is supposed to link or copy Geo.py after it creates the GeoPredicate folder

File "/Users/chupman/github/janusgraph-python/src/main/python/janusgraph_python/core/attribute/__init__.py", line 21, in <module> from .GeoPredicate import Geo ImportError: cannot import name 'Geo'

On CentOS
It is defaulting to python 2. I was able to get it to run with python3, but ran into different errors. I put the output in a GIST

I'm seeing the test errors as well on Linux and OSX when using Python 3.6. I can also reproduce with Docker.

$ docker run --rm -v $PWD:/code -w /code -it python:3.6 bash build.sh venv Collecting virtualenv Downloading https://files.pythonhosted.org/packages/b6/30/96a02b2287098b23b875bc8c2f58071c35d2efe84f747b64d523721dc2b5/virtualenv-16.0.0-py2.py3-none-any.whl (1.9MB) 100% |████████████████████████████████| 1.9MB 17.8MB/s Installing collected packages: virtualenv Successfully installed virtualenv-16.0.0 Using base prefix '/usr/local' New python executable in /code/tempENV/bin/python Installing setuptools, pip, wheel...done. You are using pip version 10.0.1, however version 18.1 is available. You should consider upgrading via the 'pip install --upgrade pip' command. Building documentation Python 3.6.6 PyBuilder version 0.11.17 Build started at 2018-10-17 22:21:49 ------------------------------------------------------------ [INFO] Building janusgraph_python version 0.0.9 [INFO] Executing build in /code [INFO] Going to execute task sphinx_generate_documentation [INFO] Installing plugin dependency coverage [INFO] Installing plugin dependency pypandoc [INFO] Installing plugin dependency unittest-xml-reporting [INFO] Running sphinx_html ------------------------------------------------------------ BUILD SUCCESSFUL ------------------------------------------------------------ Build Summary Project: janusgraph_python Version: 0.0.9 Base directory: /code Environments: Tasks: prepare [4016 ms] sphinx_generate_documentation [1787 ms] Build finished at 2018-10-17 22:21:55 Build took 5 seconds (5850 ms) Building library Python 3.6.6 PyBuilder version 0.11.17 Build started at 2018-10-17 22:21:56 ------------------------------------------------------------ [INFO] Building janusgraph_python version 0.0.9 [INFO] Executing build in /code [INFO] Going to execute tasks: clean, install_dependencies, prepare, compile_sources, package, publish [INFO] Removing target directory /code/target [INFO] Installing all dependencies [INFO] Processing batch dependency 'gremlinpython==3.3.3' [INFO] Running unit tests [WARN] unittest_file_suffix is deprecated, please use unittest_module_glob [INFO] Executing unit tests from Python modules in /code/src/unittest/python [INFO] Executed 10 unit tests [ERROR] Test has error: unittest.loader._FailedTest.Point_test [ERROR] Test has error: unittest.loader._FailedTest.Circle_test [ERROR] Test has error: unittest.loader._FailedTest.RelationIdentifier_test [ERROR] Test has error: unittest.loader._FailedTest.GeoShapeDeserializer_test [ERROR] Test has error: unittest.loader._FailedTest.PointSerializer_test [ERROR] Test has error: unittest.loader._FailedTest.CircleSerializer_test [ERROR] Test has error: unittest.loader._FailedTest.RelationIdentifierSerializer_test [ERROR] Test has error: unittest.loader._FailedTest.RelationIdentifierDeserializer_test [ERROR] Test has error: unittest.loader._FailedTest.GraphsonWriter_test [ERROR] Test has error: unittest.loader._FailedTest.GraphsonReader_test ------------------------------------------------------------ BUILD FAILED - There were 10 error(s) and 0 failure(s) in unit tests ------------------------------------------------------------ Build finished at 2018-10-17 22:21:59 Build took 3 seconds (3407 ms)...

@sjudeng @chupman : The build failures were part of me incorporating few of comments in this PR. My bad. I got access to few Ubuntu and CentOS machines, and I've updated this PR with the fixes for the build fauilres. I just tested on Ubuntu and CentOS and both works file, can you guys confirm the same?

Also, the usage of build script has changed to following:
/build.sh -d true -b true -i true -p 3.6
Where the parameter -p corresponds to python version you have installed on your system. It can be verified by running either of following commands python3/python3.4/python3.5/python3.6 whichever is installed, you pass accordingly. I've updated the docs for the same.

pulled the changes and it built fine on my mac. Here's the output with the test warnings.

Build finished at 2018-10-19 19:26:51 Build took 4 seconds (4497 ms) Building library Python 3.6.6 PyBuilder version 0.11.17 Build started at 2018-10-19 19:26:51 ------------------------------------------------------------ [INFO] Building janusgraph_python version 0.0.9 [INFO] Executing build in /Users/chupman/github/janusgraph-python [INFO] Going to execute tasks: clean, install_dependencies, prepare, compile_sources, package, publish [INFO] Removing target directory /Users/chupman/github/janusgraph-python/target [INFO] Installing all dependencies [INFO] Processing batch dependency 'gremlinpython==3.3.3' [INFO] Running unit tests [WARN] unittest_file_suffix is deprecated, please use unittest_module_glob [INFO] Executing unit tests from Python modules in /Users/chupman/github/janusgraph-python/src/unittest/python [INFO] Executed 19 unit tests [INFO] All unit tests passed. [INFO] Building distribution in /Users/chupman/github/janusgraph-python/target/dist/janusgraph_python [INFO] Copying scripts to /Users/chupman/github/janusgraph-python/target/dist/janusgraph_python/scripts [INFO] Writing setup.py as /Users/chupman/github/janusgraph-python/target/dist/janusgraph_python/setup.py [INFO] Collecting coverage information [INFO] Running unit tests [WARN] unittest_file_suffix is deprecated, please use unittest_module_glob [INFO] Executing unit tests from Python modules in /Users/chupman/github/janusgraph-python/src/unittest/python [INFO] Executed 19 unit tests [INFO] All unit tests passed. [WARN] Module 'janusgraph_python.core.attribute.GeoPredicate.Geo' was not imported by the covered tests [WARN] Coverage for module 'janusgraph_python.core.attribute.GeoPredicate.Geo' cannot be established - module failed: No module named 'janusgraph_python.core.attribute.datatypes' [WARN] Module 'janusgraph_python.core.attribute.TextPredicate.Text' was not imported by the covered tests [WARN] Module 'janusgraph_python.driver' was not imported by the covered tests [WARN] Branch coverage below 60% for janusgraph_python.core.datatypes.RelationIdentifier: 50% [WARN] Test coverage below 50% for janusgraph_python.driver.ClientBuilder: 0% [WARN] Branch coverage below 60% for janusgraph_python.driver.ClientBuilder: 0% [INFO] Overall coverage is 84% [INFO] Overall coverage branch coverage is 62% [INFO] Overall coverage partial branch coverage is 83% [INFO] Building binary distribution in /Users/chupman/github/janusgraph-python/target/dist/janusgraph_python ------------------------------------------------------------ BUILD SUCCESSFUL

chupman · 2018-10-11T18:41:10Z

build.sh

+if [ "${build}" == "true" ]
+then
+  echo "Building library"
+  chmod +x build.sh


this should be chmod +x build-library.sh

Somehow your comments didn't show up earlier. I've fixed this in recent commit. Also, can you try again now, and also based on my prev comments, and check for your python version @chupman ?

chupman · 2018-10-22T23:25:06Z

src/main/python/janusgraph_python/core/datatypes/Circle.py

+        Args:
+            latitude (float): The latitude of circle's center.
+            longitude (float): The longitude of circle's center
+            radiusInKM (int): The radius of circle in KM.


Radius should be a float as well. From the JavaDoc. It's already been mentioned, but it should be latitude, longitude

public static Geoshape circle(double latitude, double longitude, double radiusInKM)

Thanks for the pointer. Didn't know we needed to make radius also in float. I'll push a commit with the same change.
As for order of latitude and longitude being replaced, well I've issue #10 to track the same. The fix is ready, but is going to be part of next 0.2.0 release along with schema managmenet. Once that is out, I'll merge the same downstream to this PR.

chupman · 2018-10-22T23:26:06Z

src/main/python/janusgraph_python/core/datatypes/Circle.py

+
+        self.latitude = float(latitude) * 1.0
+        self.longitude = float(longitude) * 1.0
+        self.radius = radiusInKM


same as above. This should be:

self.radius = float(radiusInKM) * 1.0

FlorianHockmann · 2018-11-26T11:31:14Z

@debasishdebs You have made >50 commits since the last comments here. Is this PR now in a state where you would consider it ready? I'm just asking because I lost track here a bit and wonder whether it makes sense to review this again.

debasishdebs · 2018-11-26T18:37:58Z

@FlorianHockmann : Thanks for bringing this up. Even I had lost track of this PR due to other engagements. I've addressed all the comments by reviewers, but you can do one final overview and see if yours are addressed or not. I would still say this PR is almost complete, and docs are updated accordingly too.

@pluradj @sjudeng @chupman @mbrukman can you guys confirm the same? Its been too long this PR has remained open, and I'm also almost ready with version 2 of client incorporating Schema management.

chupman · 2018-12-03T21:19:42Z

I was able to build on osx with sudo -H ./build.sh, I would add this to the documentation.

Looks like all the tests are passing and it's just warning about coverage. I'll still need to try out the geoWithin and textsearch APIs.

Building library
Python 3.6.6
PyBuilder version 0.11.17
Build started at 2018-12-03 12:35:47
------------------------------------------------------------
[INFO]  Building janusgraph_python version 0.0.9
[INFO]  Executing build in /Users/chupman/github/janusgraph-python
[INFO]  Going to execute tasks: clean, install_dependencies, prepare, compile_sources, package, publish
[INFO]  Removing target directory /Users/chupman/github/janusgraph-python/target
[INFO]  Installing all dependencies
[INFO]  Processing batch dependency 'gremlinpython==3.3.3'
[INFO]  Running unit tests
[WARN]  unittest_file_suffix is deprecated, please use unittest_module_glob
[INFO]  Executing unit tests from Python modules in /Users/chupman/github/janusgraph-python/src/unittest/python
[INFO]  Executed 19 unit tests
[INFO]  All unit tests passed.
[INFO]  Building distribution in /Users/chupman/github/janusgraph-python/target/dist/janusgraph_python
[INFO]  Copying scripts to /Users/chupman/github/janusgraph-python/target/dist/janusgraph_python/scripts
[INFO]  Writing setup.py as /Users/chupman/github/janusgraph-python/target/dist/janusgraph_python/setup.py
[INFO]  Collecting coverage information
[INFO]  Running unit tests
[WARN]  unittest_file_suffix is deprecated, please use unittest_module_glob
[INFO]  Executing unit tests from Python modules in /Users/chupman/github/janusgraph-python/src/unittest/python
[INFO]  Executed 19 unit tests
[INFO]  All unit tests passed.
[WARN]  Module 'janusgraph_python.core.attribute.GeoPredicate.Geo' was not imported by the covered tests
[WARN]  Coverage for module 'janusgraph_python.core.attribute.GeoPredicate.Geo' cannot be established - module failed: No module named 'janusgraph_python.core.attribute.datatypes'
[WARN]  Module 'janusgraph_python.core.attribute.TextPredicate.Text' was not imported by the covered tests
[WARN]  Module 'janusgraph_python.driver' was not imported by the covered tests
[WARN]  Branch coverage below 60% for janusgraph_python.core.datatypes.RelationIdentifier: 50%
[WARN]  Test coverage below 50% for janusgraph_python.driver.ClientBuilder:  0%
[WARN]  Branch coverage below 60% for janusgraph_python.driver.ClientBuilder:  0%
[INFO]  Overall coverage is 84%
[INFO]  Overall coverage branch coverage is 62%
[INFO]  Overall coverage partial branch coverage is 83%
[INFO]  Building binary distribution in /Users/chupman/github/janusgraph-python/target/dist/janusgraph_python
------------------------------------------------------------
BUILD SUCCESSFUL

chupman

I'm still having issues with geoWithin and geoContains, but it could be me using it wrong so I'll play around a bit more. That said I did find some deadlinks in the generated docs
In the file janusgraph-python/docs/_build/connecting.html
under Register Custom Serializer/Deserializer
the Point, Circle and RelationIdentifier links are invalid as well as the see docs link

Also if you setup a connection with the graph parameter, like below, you'll get an error that you have to set graphson_reader and graphson_writer
client = JanusGraphClient().connect(url="0.0.0.0", port="8182", graph="airroutes_traversal")

pluradj

Thanks for working on this @debasishdebs . Some more review comments, mostly minor.

docs/installation.rst

pluradj · 2018-12-10T22:19:49Z

.travis.yml

@@ -30,15 +30,15 @@ install:

 before_script:
  # Install all pre-requisites for building library and docs
-  - bash before_script.sh ${ENV_NAME}
+  - bash before-script.sh ${ENV_NAME}


these don't appear to be fixed yet in this PR

pluradj · 2018-12-10T22:23:24Z

requirements.txt

@@ -0,0 +1 @@
+dist/janusgraph_python-0.0.9.tar.gz


so you'll need to remove dist/janusgraph_python-0.0.9.tar.gz before merge

pluradj · 2018-12-10T22:34:05Z

docs/installation.rst

+
+    pip install janusgraph-python==x.y.z
+    # Where x.y.z is version number, corresponding to JanusGraph version you are using.
+    # Please refer to compatibility matrix to get version compatible against each JanusGraph version


Where is the compatibility matrix for the JanusGraph-Python driver? Or is the example below meant to be the matrix? I think this should mention specifically whether this driver supports JanusGraph 0.2.x and/or 0.3.x.

pluradj · 2018-12-13T20:55:07Z

.gitignore

+.env
+
+# mkdocs documentation
+/site


probably should be site/ or */site/

pluradj · 2018-12-13T21:45:26Z

docs/geo-shapes.rst

+This data type / Geometric object is part of GeoShape package.
+It is equivalent to Geometric Circle defined by Latitude, Longitude and Radius.
+
+**NOTE, the Radius is in KMs**


nit: a bit of inconsistency here in the **Note usage compared with installation.rst.

pluradj · 2018-12-13T21:46:58Z

docs/contributing.rst

+Contributing to JanusGraph-Python
+===================================
+
+This page is under development, and will be updated soon.


Could copy the same text for contributing from the README.md into here. Or you could leave this page out instead of duplicating.

pluradj · 2018-12-13T21:49:58Z

BUILDING.md

+
+     - And so on depending on your requirements.
+
+Once done, you can see the built HTML files under `docs/_build/index.html` directory.


Also mention where the results of the library build end up in the file hierarchy.

pluradj · 2018-12-13T22:30:59Z

src/main/python/janusgraph_python/driver/ClientBuilder.py

+        self.graphsonVersion = version
+        pass
+
+    def connect(self, url="loclahost", port="8182", graph="g", **kwargs):


s/loclahost/localhost
Also url should be host

pluradj · 2018-12-13T22:40:40Z

docs/connecting.rst

+    reader_builder = JanusGraphSONReader().register_deserializer(geoshape_identifier, geoshape_deserializer)
+    reader = reader_builder.get()
+    writer_builder = JanusGraphSONWriter().register_serializer(obj_to_register, circle_serializer)
+    writer = writer_builder.get()


I find this section a bit confusing. JanusGraphSONReader and JanusGraphSONWriter already do default registrations for the Geo datatypes. It might be better to use fictional custom types instead.

debasishdebs · 2018-12-29T15:31:41Z

@chupman @pluradj : Let me know if your comments are resolved. Also, I've added comparability matrix as part of README, so that it id directly visible to user, removed redundant files, and also fixed #10 .

chupman

I tested the latest changes after updating the changes I mentioned in ClientBuilder and the Text and Geo predicates are both working. Latitude and longitude have been swapped as requested as well.

Awesome work on this, I think it's getting close. Thanks for sticking with it through all the iterations.

.gitignore

before-script.sh

build-docs.sh

build-library.sh

chupman · 2019-01-02T18:02:31Z

src/main/python/janusgraph_python/driver/ClientBuilder.py

+        self.graphsonVersion = version
+        pass
+
+    def connect(self, host="loclahost", port="8182", graph="g", **kwargs):


This should be:

def connect(self, url="localhost", port="8182", graph="g", **kwargs):

Agree on the misspelled localhost. I requested the change from url to host previously, as noted in the parameter doc below.

My bad. Latest commit fixes this.

pluradj · 2019-01-02T18:35:53Z

src/main/python/janusgraph_python/driver/ClientBuilder.py

+            JanusGraphClient
+        """
+
+        URL = "ws://{}:{}/gremlin".format(url, port)


Should be .format(host, port)

pluradj · 2019-01-02T18:37:18Z

src/main/python/janusgraph_python/driver/ClientBuilder.py

+            Connect to JanusGraph's gremlin-server instance. Takes URL, Port and Graph
+
+        Args:
+            host (str): The URL of JanusGraph gremlin-server instance. Default to localhost


Should be host (str): The HOST of JanusGraph...

mbrukman · 2019-01-07T00:39:54Z

README.md

@@ -1,6 +1,77 @@
-# JanusGraph Python driver
+# JanusGraph-Python
+### Python Client drivers for [JanusGraph](janusgraph.org) 


http://janusgraph.org

(without http, it looks like a relative file path, and will become a broken link)

README.md

mbrukman · 2019-01-07T00:41:48Z

build.cmd

@@ -0,0 +1,76 @@
+@echo off


Please add a copyright header to the top of this file.

mbrukman · 2019-01-07T00:41:58Z

build-library.sh

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+


Drop extra blank line here.

mbrukman · 2019-01-07T00:42:35Z

build.cmd

+    cd ..\
+
+    call deactivate
+


Remove extra blank line, please.

mbrukman · 2019-01-07T02:02:06Z

BUILDING.md

+
+- Building on Unix System:
+    - A script named build.sh is created which accepts following parameters:
+        - -d: Default to true. Implies, if documentation is to be build.


Put -d in backticks so that it's formatted as code; similarly for the flags below.

mbrukman · 2019-01-07T02:02:34Z

BUILDING.md

+
+- Building on Unix System:
+    - A script named build.sh is created which accepts following parameters:
+        - -d: Default to true. Implies, if documentation is to be build.


Drop "implies", just say it "specifies whether the docs should be built."

Similarly for the next several lines.

mbrukman · 2019-01-07T02:03:24Z

BUILDING.md

+    - If any of parameters is missing, the script exits with error message. 
+    An appropriate way to invoke the script would be:
+        ```bash
+        build.cmd "d=true" "b=true" "i=true"


Is this standard for Windows commands to use =true or =false to specify flags? Doesn't Windows use /foo flag format?

@mbrukman : This is first time I'm building any similar sort of shell scripts for windows and unix. I just tried searching for how to implement keyword arguments for batch file, and this method seems to be working. If we want to change to another flag format, then we can either create a issue to track the same which will take care of changing the keyword arguments for both Windows batch as well as Unix shell files. What would you say?

mbrukman · 2019-01-07T02:05:54Z

BUILDING.md

+
+     - If you want to build just the documentation, without the library.
+        ```bash
+        ./build.sh -b false


This is not the typical way that Unix/Linux command line flags are written. It's typically -b implies true, and --no-b or --without-b implies false.

Can we change this to:

-d, --docs: build docs

--no-docs: does not build docs

-l, --library: build library

--no-library: does not build library
etc.

Thoughts?

Please see my my previous comment.

mbrukman · 2019-01-07T02:06:21Z

README.md

+
+#### Pre-Requisites
+
+The library has been tested with following Python versions:


Does this library require Python3? Do we not plan to have Python2 compatibility at all?

Should that be documented here?

Well, for building this library for Python2, we will need to do a lot of changes to current library. I was thinking, first let us have a version released supporting Python3, and depending on how community responds, and looking as weather there is requirement for Python2, we can then take a call on weather to support Python2. What would be your thoughts?

I don't think we need Python 2 with this PR. Document the specific Python 3 versions this does currently support, and leave Python 2 for a separate issue if there is enough interest from the community.

So we need to menton python version till 3 digits? Or just 3.6 is okay instead of mentioning like 3.6.5, 3.6.6, etc @pluradj ?

debasishdebs · 2019-03-27T09:51:41Z

Also, closing out #15 #14 #13 #11 #10 #8 #7 #6 #5 while #9 #11 #12 #17 #18 #19 remains open. #1 will be closed once PR in merged.

Signed-off-by: Debasish Kanhar <[email protected]>

debasishdebs · 2019-03-27T10:57:52Z

Okay guys help here. @FlorianHockmann @chupman @pluradj @mbrukman @sjudeng : I squashed all commits into one on local using git rebase --root -i. I pick just the first and squash all others. There were few conflicts but they were resolved.

Once I squashed commits on local, I force pushed them using git push origin fix_#3 --force and somehow that closed out this PR. Now I'm not even able to open it again. Did I do something wrong? What should I do?

Thanks

chupman · 2019-03-27T20:28:28Z

@debasishdebs you squashed a little bit too much. There are 4 commits that should have been included from the project setup. You probably want to do a git reflog to undo your rebase and then only squash your commits.

sjudeng · 2019-03-28T00:09:46Z

You could also do a soft reset to origin/master and recommit

git checkout -b fix_#3_new debasishdebs/fix_#3
git fetch origin
git reset --soft origin/master
git commit -s -m "Initial commit"
git push debasishdebs fix_#3_new:fix_#3 --force

debasishdebs · 2019-03-28T11:18:22Z

@sjudeng : Thanks for the idea. Though your method didn't work as you suggested, and I had to do few changes to make that work. I was able to create a new branch as fix_#3_new and made it track fix_#3. I've pushed the new branch, and created a new PR #20 . But wouldn't it be same as me creating new branch, pushing it and creating a new PR?

I was expecting the existing PR to re-open but that didn't happen and hence had to create a new PR with all codes updated based on comments I've recieved as part of this PR.

sjudeng · 2019-03-28T12:45:00Z

Agreed. I'm not sure why this wouldn't reopen. Creating the separate PR might be all you could do at this point.

chupman · 2019-03-28T16:00:24Z

Opening a new PR was definitely the easier path (and with less Unicorns), but I found this gist that describes reopening a PR that closes due to a disconnect in case you guys are curious.

debasishdebs · 2019-04-01T06:39:40Z

Thanks guys for all the help @chupman @sjudeng . Well once I created the new PR, I've been out of sync as I don't get any notifications. But I'll follow those, and I'll try to give as much as time to move this over to finish line.

Thanks for moving my first ever library towards finish line. Let us hope now it helps so many users of JanusGraph who use Python as primary development language. :-)

janusgraph-bot added the cla: yes label Sep 4, 2018

debasishdebs mentioned this pull request Sep 4, 2018

Adding Initial version of JanusGraph Python Client #2

Closed

pluradj changed the title ~~Fixing CLA signs.~~ Initial version of JanusGraph Python client drivers Sep 4, 2018

sjudeng reviewed Sep 5, 2018

View reviewed changes

janusgraph-bot added the cla: yes label Sep 5, 2018

FlorianHockmann reviewed Sep 6, 2018

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

build.py Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

mbrukman requested changes Sep 7, 2018

View reviewed changes

pluradj requested changes Sep 7, 2018

View reviewed changes

mbrukman previously approved these changes Sep 9, 2018

View reviewed changes

pluradj reviewed Sep 10, 2018

View reviewed changes

README.md Show resolved Hide resolved

mbrukman reviewed Sep 13, 2018

View reviewed changes

debasishdebs added a commit to debasishdebs/janusgraph-python that referenced this pull request Sep 24, 2018

Merge pull request JanusGraph#7 from debasishdebs/dynamic_tinkerpop_#4

c557940

Dynamic tinkerpop JanusGraph#4

chupman requested changes Oct 3, 2018

View reviewed changes

FlorianHockmann requested changes Oct 5, 2018

View reviewed changes

chupman reviewed Oct 10, 2018

View reviewed changes

chupman reviewed Oct 11, 2018

View reviewed changes

chupman reviewed Oct 22, 2018

View reviewed changes

chupman requested changes Dec 4, 2018

View reviewed changes

pluradj reviewed Dec 13, 2018

View reviewed changes

chupman requested changes Jan 2, 2019

View reviewed changes

pluradj reviewed Jan 2, 2019

View reviewed changes

mbrukman requested changes Jan 7, 2019

View reviewed changes

JanusGraph deleted a comment Mar 27, 2019

Initial commit. (Squashed)

5f9813a

Signed-off-by: Debasish Kanhar <[email protected]>

debasishdebs closed this Mar 27, 2019

debasishdebs force-pushed the fix_#3 branch from 0f1c223 to 5f9813a Compare March 27, 2019 10:53


		For installing PyBuilder, the following needs to be run:

		```pip install pybuilder```


		Once PyBuilder is installed, run the following commands to build the Library:

		```pyb```

		@@ -6,3 +6,36 @@ JanusGraph Python driver code is provided under the [Apache 2.0
		license](APACHE-2.0.txt) and documentation is provided under the [CC-BY-4.0



		For building the library, the pre requisite is that PyBuilder needs to be installed on system.



		class Circle(object):
		def __init__(self, longitude, latitude, radiusInKM):

		- [Python 3.6](https://www.python.org/download/releases/3.6.0/).

		Once the required `Python` version is installed on system, the following requirements needs

		```

		Once done, you can see the build HTML files under `docs/_build` directory.


		- Building Library:
		- To build the library run the following command:



		class Point(object):
		def __init__(self, longitude, latitude):

		from janusgraph_python.core.datatypes.GeoShape import GeoShape


		class GeoContains(object):


		# mypy
		.mypy_cache/

		@@ -0,0 +1 @@
		dist/janusgraph_python-0.0.9.tar.gz


		- And so on depending on your requirements.

		Once done, you can see the built HTML files under `docs/_build/index.html` directory.

		# See the License for the specific language governing permissions and
		# limitations under the License.


		#### Pre-Requisites

		The library has been tested with following Python versions:

Initial version of JanusGraph Python client drivers #4

Initial version of JanusGraph Python client drivers #4

Conversation

debasishdebs commented Sep 4, 2018 • edited Loading

sjudeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlorianHockmann left a comment

Choose a reason for hiding this comment

debasishdebs commented Sep 6, 2018 • edited Loading

mbrukman 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

mbrukman Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

debasishdebs Sep 10, 2018 • 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

Choose a reason for hiding this comment

pluradj 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

debasishdebs Sep 8, 2018 • 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

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

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

chupman left a comment • 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

FlorianHockmann 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

debasishdebs commented Sep 4, 2018 •

edited

Loading

debasishdebs commented Sep 6, 2018 •

edited

Loading

mbrukman Sep 7, 2018 •

edited

Loading

debasishdebs Sep 10, 2018 •

edited

Loading

debasishdebs Sep 8, 2018 •

edited

Loading

chupman left a comment •

edited

Loading