pycsw

[tomkralidis]

This PR has the following assumptions:

• PR ResourceBase implementation #359 is merged
• geonode.catalogue is part of INSTALLED_APPS
• pycsw is enabled as the default CSW endpoint
• pycsw is bound directly to the GeoNode DB (pycsw_local)
• pycsw is embedded in GeoNode

[tomkralidis]

First point: Shapely brings a GEOS dependency. GDAL is not required by Shapely or pycsw. GDAL may be built with GEOS to enable spatial predicate support in GDAL.

[tomkralidis]

Second point: the only outstanding item is your request to put csw specific fields in catalogue.models and OneToOneField them in geonode.layers.models.ResourceBase. You can see the impl at http://dpaste.de/CoC1c/. When running paver start, I get the following traceback: http://dpaste.de/BhROz/ . Note sure what gives here. Any sugestions?

Thanks

[ischneider]

This is a circular dependency error. A brief inspection makes me think:

1. ResourceBase needs extraction from the geonode.layers.models to elsewhere - either geonode.catalogue or perhaps a base module located elsewhere
2. the catalogue.models module looks suspect - there are no models there, just signal connections and handlers. Perhaps extract the handlers and register the connections in the respective models (if the condition applies)?

On a separate note, this also looks suspect - https://github.com/tomkralidis/geonode/blob/pycsw/geonode/catalogue/models.py#L78
The signal is already connected (unless I'm missing something)

[tomkralidis]

@ischneider thanks for the info. That signal is previously disconnected: https://github.com/tomkralidis/geonode/blob/pycsw/geonode/catalogue/models.py#L68

[ischneider]

@tomkralidis d'oh - just saw that quickly but didn't look at the context :)

[ingenieroariel]

The need to connect and disconnect things there is another reason why that
information would be better stored in a related model. Look at the way the
links are created, using that approach there is no need to save again the
instance.

Also note that when you re-save the geoserver and other signals do get
triggered. I will try to take a crack at this problem over the weekend
since I have a strong interest on not having to save an instance twice (
like we are doing in master [1])

Ariel

[1]
https://github.com/GeoNode/geonode/blob/master/src/GeoNodePy/geonode/maps/models.py#L1644

On Thu, Sep 6, 2012 at 8:44 PM, Ian Schneider [email protected]:

@tomkralidis https://github.com/tomkralidis d'oh - just saw that
quickly but didn't look at the context :)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/387#issuecomment-8352565.

[ingenieroariel]

On this,

I am strongly opposed on having layers/models.py import anything from
catalogue (that also goes for layers/utils.py).

The two main reasons are avoiding this kind of circular dependencies and
the possibility of making the catalogue optional, it also leads to easier
to debug code.

Since the only thing missing is a django signals / models reorg. I can take
care of that and merge the pull request over the weekend.

On Fri, Sep 7, 2012 at 8:45 AM, Ariel Nunez [email protected]:

The need to connect and disconnect things there is another reason why that
information would be better stored in a related model. Look at the way the
links are created, using that approach there is no need to save again the
instance.

Also note that when you re-save the geoserver and other signals do get
triggered. I will try to take a crack at this problem over the weekend
since I have a strong interest on not having to save an instance twice (
like we are doing in master [1])

Ariel

[1]
https://github.com/GeoNode/geonode/blob/master/src/GeoNodePy/geonode/maps/models.py#L1644

On Thu, Sep 6, 2012 at 8:44 PM, Ian Schneider [email protected]:

@tomkralidis https://github.com/tomkralidis d'oh - just saw that
quickly but didn't look at the context :)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/387#issuecomment-8352565.

[ischneider]

@ingenieroariel , given the large history w/ mostly trivial changes, when you merge, can you consider squashing this into one (or maybe a few) commit?

[ingenieroariel]

Thanks for the hint Ian, will follow your advice.

On Fri, Sep 7, 2012 at 11:46 AM, Ian Schneider [email protected]:

@ingenieroariel https://github.com/ingenieroariel , given the large
history w/ mostly trivial changes, when you merge, can you consider
squashing this into one (or maybe a few) commit?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/387#issuecomment-8369253.

[ingenieroariel]

Since there are a lot of commits interleaved with other commits in master and an automatic full rebase on dev fails, I am little bit worried on messing up the code by playing with git's history.

BTW, I was able to successfully run the unit, integration and csw tests on this branch. Will start this afternoon the work on relocating the csw fields and removing the need for a double save.

[ingenieroariel]

After giving this a long afternoon and was able to fix the OneToOne relationship, and avoid the double saving.

But

I found a roadblock with the PYCSW mappings. Where they are designed to have all attributes in just one model. Alternatives to this including getting related attributes by having pycsw check for . (dot) notation, but that does not seem worth the effort.

I was able to rebase onto dev, squash a few issues and add the missing migrations, the output branch can be found here:

https://github.com/ingenieroariel/geonode/tree/pycsw

However, I realized after more careful testing that while csw and integration tests pass, the unit tests are still failing, since they end up calling the catalogue signals and they return a failure. Once unit tests pass, my suggestion is to merge this work in, disregarding my request to take the fields out.

I am still not 100% happy with having these catalogue fields in layers/models.py and the current double saving, but do not believe getting that right is worth losing on getting pycsw as the default CSW catalogue sooner rather than later.

It would be great if other committers chime in and let us know what they think of this PR, in my humble opinion, it should go in right after unit tests pass again.

[tomkralidis]

@ingenieroariel thanks for the info. Before I comment, how can I see your branch in diff mode against geonode dev branch?

[ingenieroariel]

dev...ingenieroariel:pycsw

[tomkralidis]

@ingenieroariel : outside of the migrations, what's the difference between your branch and the branch in this PR? That will help us figure out how/why the unit tests are not working anymore (where they were in #387 (comment))

[ingenieroariel]

Tom: Apologies for not clarifying. After a few rounds of running all tests
I verified that the unit tests consistently failed in the current PR. There
are no code changes in that branch, only the rebasing and squashing.

The reason unit tests are failing are because the catalogue signals are
being attached and a lot of connection refused errors are being triggered.

In this PR (#387) as well as that branch, integration and csw pass, but
unit tests fail if the server is not running.

-a

On Tue, Sep 11, 2012 at 8:47 PM, Tom Kralidis [email protected]:

@ingenieroariel https://github.com/ingenieroariel : outside of the
migrations, what's the difference between your branch and the branch in
this PR? That will help us figure out how/why the unit tests are not
working anymore (where they were in
#387 (comment))

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/387#issuecomment-8477827.

[tomkralidis]

@ingenieroariel thanks for the info. Any suggestions on how to address this? Given that CSW is a component, should the integration and csw tests be enough?

[ingenieroariel]

" should the integration and csw tests be enough? "
Not if they break other tests when it is enabled.

The 3 options I see are:

#1. Disconnecting the catalogue signals when running the unit tests (I
would like to avoid this one if possible). (Change the tests)
#2. Changing the unit tests to the LiveTestCase that starts up and http
server when running the tests, so pycsw can be accessed during runtime.
(Change the tests)
#3. Avoid the use of http for interacting with pycsw in 'local' mode.
(Change the code)

On Wed, Sep 12, 2012 at 7:36 AM, Tom Kralidis [email protected]:

@ingenieroariel https://github.com/ingenieroariel thanks for the info.
Any suggestions on how to address this? Given that CSW is a component,
should the integration and csw tests be enough?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/387#issuecomment-8490230.

[tomkralidis]

Fixes cedb12a and 0fa4db1 result in passing unit, integration, and CSW tests, which creates get_record and search_records defs for the pycsw_local backend. These defs do get into the raw CSW response; this is done as a means to bypass OWSLib (which always uses HTTP). I also looked into just stubbing against geonode.layers.views.layer_search, but that one needed a Django request object, hence this fix.

@ingenieroariel is this okay?

[ingenieroariel]

Awesome,

Thanks for fixing that so quickly, like I said before I am +2 on merging
this PR with dev.

-a

On Wed, Sep 12, 2012 at 12:02 PM, Tom Kralidis [email protected]:

Fixes cedb12a cedb12a and
0fa4db1 0fa4db1 result in
passing unit, integration, and CSW tests, which creates get_record and
search_records defs for the pycsw_local backend. These defs do get into
the raw CSW response; this is done as a means to bypass OWSLib (which
always uses HTTP). I also looked into just stubbing against
geonode.layers.views.layer_search, but that one needed a Django request
object, hence this fix.

@ingenieroariel https://github.com/ingenieroariel is this okay?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/387#issuecomment-8499365.