e-mission / e-mission-docs

Repository for docs and issues. If you need help, please file an issue here. Public conversations are better for open source projects than private email.
https://e-mission.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
15 stars 34 forks source link

⚡ 🚸 Re-enable populating addresses in trips and make sure that they are used on the phone #937

Open shankari opened 1 year ago

shankari commented 1 year ago

Right now, we don't fill in address information in the trips when we process them. Instead, the phone "fills in" the addresses when it receives the trips by making calls to OSM nominatim. This is annoying because we make a lot of potentially redundant calls to nominatim (although @JGreenlee) is caching some information on the phone, and it is also annoying for users because the addresses fill in one by one by one very slowly and it uses up people's data plans.

Instead, while we are filling in place information, we should fill in the addresses so that they will be part of the trip and used in multiple places without having to keep re-looking up the information. It will also have the benefit that we can know the address at the time the trip was taken, rather than the time the trip was viewed, in case there are changes to the map in the middle.

There is existing code to fill this in using nominatim on the server, but it runs into nominatim usage limits https://github.com/e-mission/e-mission-server/blob/8761f4afce635bc4cc7ff1732d9f00956cb5c4ad/emission/analysis/intake/cleaning/clean_and_resample.py#L250

However, we are in the process of signing up for a paid nominatim service. To prepare for that, we should:

shankari commented 1 year ago

So concretely:

After those are done, we can move to the phone changes.

shankari commented 1 year ago

For the record, emission.net.ext_service.geocoder.nominatim currently also uses google and falls back to nominatim if Google is not configured. We should remove the Google API implementation - we will only use nominatim going foward.

https://github.com/e-mission/e-mission-server/blob/8761f4afce635bc4cc7ff1732d9f00956cb5c4ad/emission/net/ext_service/geocoder/nominatim.py#L17

nataliejschultz commented 1 year ago

I have spent most of my time on this project so far reading and testing the functionality of nominatim.py, reading up on the nominatim api documentation, and preparing to make API calls.

I was initially unsure how to run the nominatim calls locally, since their documentation is not great. I was able to download a docker container from nominatim-docker that contains everything needed to run a localhost and make nominatim calls:

docker run -it \
  -e PBF_URL=https://download.geofabrik.de/europe/monaco-latest.osm.pbf \
  -e REPLICATION_URL=https://download.geofabrik.de/europe/monaco-updates/ \
  -p 8080:8080 \
  --name nominatim \
  mediagis/nominatim:4.2

I then modified nominatim.json to say:

"query_url" : "http://localhost:8080/nominatim/"

per this exchange.

When trying to run, I kept getting the following error:

google maps key not configured, falling back to nominatim
http://localhost:8080/nominatim
>>> reverse_geocoded_json = eco.Geocoder.get_json_reverse("39.4818456", "-106.0445699")
<urllib.request.Request object at 0x100bb5940>
>>> print(reverse_geocoded_json)
{'error': 'Unable to geocode'}

I decided to look through old files since @JGreenlee showed me how (thank you!!!) and found out that I could just use "query_url" : "http://nominatim.openstreetmap.org" to run my small query. My call worked after that.

shankari commented 1 year ago

I was initially unsure how to run the nominatim calls locally, since their documentation is not great.

I'm glad you figured this out - just to clarify:

We can use the free nominatim service to check this, as long as we only make a few calls an hour (https://operations.osmfoundation.org/policies/nominatim/)

so you didn't have to run the nominatim service locally - just make calls from your laptop to the free nominatim service (nominatim.openstreetmap.org)

wrt

>>> reverse_geocoded_json = eco.Geocoder.get_json_reverse("39.4818456", "-106.0445699")

Do you know why it didn't work? Hint: Look at the data that you downloaded into your container.

shankari commented 1 year ago

Answer: because we loaded data from Monaco but were trying to access a location in Colorado. To verify, you can:

shankari commented 1 year ago

our integration tests are currently in emission/integrationTests/ you should be able to use them as an example we should initially configure to use the standard OSM-based nominatim, but once we get the geofabrik API key, we can also configure to use it with a github actions secret configuration.

Let's first write the test and then see whether we want to use the secrets or not. we might want to run the tests against both geofabrik and OSM nominatim initially to see if they are consistent . If they stay consistent for a while, we could consider removing one.

shankari commented 1 year ago

Right now, the URL for the nominatim service is in a config file (conf/net/ext_service/nominatim.json.sample). This is going to make it tricky to configure through secrets since the secrets are made available as environment variables in the github action. Since we are rewriting this code anyway, we might want to switch to reading from an environment variable anyway.

We could also configure the file using an environment variable, but I think that doesn't make as much sense given that:

nataliejschultz commented 1 year ago

I decided to test the functionality of the docker containter that I downloaded. I started the docker container and modified the nominatim.json with this line: "query_url" : "http://localhost:8080/nominatim". In the REPL:

>>>reverse_geocoded_json = eco.Geocoder.get_json_reverse(43.729325, 7.416883) 
http://nominatim.openstreetmap.org/reverse?lat=43.729325&lon=7.416883&format=json 
>>> print(reverse_geocoded_json)
{'place_id': 64027309, 'licence': 'Data © OpenStreetMap contributors, ODbL 1.0. https://osm.org/copyright', 'osm_type': 'node', 'osm_id': 5918204054, 'lat': '43.7287949', 'lon': '7.417052', 'display_name': 'NOEUD 1987, Galerie Princesse Stéphanie, Fontvieille, Monaco, 98020, Monaco', 'address': {'tourism': 'NOEUD 1987', 'road': 'Galerie Princesse Stéphanie', 'suburb': 'Fontvieille', 'ISO3166-2-lvl10': 'MC-FO', 'city': 'Monaco', 'postcode': '98020', 'country': 'Monaco', 'country_code': 'mc'}, 'boundingbox': ['43.7287449', '43.7288449', '7.417002', '7.417102']}

I used a latitude and longitude in Monaco. I also modified nominatim.py with print(NOMINATIM_QUERY_URL ) to see how the url was being formatted.

The output was as expected. So, if the correct dataset is downloaded, the docker container method with nominatim could be useful.

nataliejschultz commented 1 year ago

Looked heavily into nominatim's documentation to see if we can query data from previous dates. Unfortunately, this is not possible for any call in nominatim.

As an alternative solution, tried to use overpy to query for the previous display name data and it did not work. It is possible to query past data using Overpass API by setting a date variable at the top:

[date:"2016-01-01T00:00:00Z"]

Resulting outputs were very large, and require a large amount of resources just to potentially get the display name out. You cannot query against addresses, unless they are very specific, which would require a large amount of looping over data. Additionally, you cannot query against lat/long, as it wants an input of bbox. Bbox outputs from our cleaned data were conflicting with requirements from the api.

shankari commented 1 year ago
  1. we should file an issue/feature request about this with the nominatim team. Note that Nominatim is open source https://github.com/osm-search/Nominatim
  2. I don't think we need to explore the overpy approach for production in general because we will query ~ 1 hour after the trip is done so we will not get bad data
  3. the real need for historical queries is: a. in case we reset the pipeline and re-run it many months after the fact b. testing

For 3 (a), we could actually use the overpy approach as a fallback if the location that we are lookup was older than a week or something.

For 3 (b) it doesn't make sense to only test the overpy to get the value if we are going to continue using nominatim as the default. If we are going to use nomimatim, we should test nominatim. So for testing alone, I would suggest that we:

For docker:

For the overpy approach, I don't see what the issue is.

If we implement the overpass python backup option, we should test that as well.

shankari commented 1 year ago

to run integration tests, you would:

So similar to the existing docker-compose but with the addition of the nominatim service.

Couple of caveats to note are:

nataliejschultz commented 1 year ago

During our meeting, we wanted to test that the original docker-compose command was working locally, so that the very basics of setting up a new container would not be an issue. While running docker-compose -f setup/docker-compose.tests.yml up in terminal, we ran into the error ./runAllTests.sh: line 9: python: command not found. Originally, we thought this was an issue of the files not copying correctly. However, the key is that it is the python command that was not found, not the file.

Line 9 ofrunAllTests.sh reads as follows:

PYTHONPATH=. python -m unittest discover -s emission/tests -p Test*;

This resulted in a python command not found error, because my machine uses python3. Changing the command to PYTHONPATH=. python3 ... fixed the error and allowed docker-compose to succeed. I may be able to fix future python incompatibilities by setting alias python='python3' in my .bash_profile.

nataliejschultz commented 1 year ago

Locally, running the following block of code successfully downloads the OSM data from Northern California and creates a container named norcal-nominatim:

docker run -it \
  -e PBF_URL=https://download.geofabrik.de/north-america/us/california/norcal-latest.osm.pbf \
  -e REPLICATION_URL=https://download.geofabrik.de/north-america/us/california/norcal-updates/ \
  -e IMPORT_WIKIPEDIA=false \
  -v nominatim-data:/var/lib/postgresql/14/main \
  -p 8080:8080 \
  --name norcal-nominatim \
  mediagis/nominatim:4.2

It also creates a volume, so that the container can be started and stopped while maintaining the data.

This command line call is great for local testing, but our current digital server uses docker-compose to test. So, I converted the commands passed into docker run -it into commands for the docker-compose file:

  nominatim:
    image: mediagis/nominatim:4.2
    container_name: norcal-nominatim
    environment:
      - PBF_URL=https://download.geofabrik.de/north-america/us/california/norcal-latest.osm.pbf
      - REPLICATION_URL=https://download.geofabrik.de/north-america/us/california/norcal-updates/
      - IMPORT_WIKIPEDIA=false
    ports:
      - "8080:8080"
    deploy:
      replicas: 1
      restart_policy:
        condition: on-failure
    volumes:
      - nominatim-data:/var/lib/postgresql/14/main
    networks:
      - emission

This way, we can integrate running the nominatim server in tandem with our mongoDB, so that we can make calls to both at the same time. This might be necessary, depending on if the other integration tests will require calls to mongo.

To run the tests, I added a directory to the integration tests called nominatimTests. I created a file called nominatim-docker-test.yml, that runs the test every Sunday at 4:05am. Most notably, it builds a Dockerfile in the integrationTests directory:

web-server:
    build: ../../integrationTests

and runs docker-compose:

name: Initialize nominatim 
run: docker-compose -f docker-compose.yml up --exit-code-from web-server

The Dockerfile runs a file called start_integration_tests.sh, which will initialize all of the integration tests.

nataliejschultz commented 1 year ago

On my test run, the servers were initializing (though seemed to be dueling due to using the same port). I tried to query the local nomination server, and it did not work. I decided to do a quick sanity check and make sure that my goal was possible by following the procedure on the nomainatim-docker page for Monaco. I ran their docker run -it and called the server using their suggested command: curl -v http://localhost:8080/search.php?q=avenue%20pasteur and received a response successfully.

I decided to try a local test using the Northern California container, but modify the common by including the optional wikipedia import, while removing the creation of the volume. This way, it would be more similar to the command used in the successful Monaco example. This increased the time required to complete the docker run -it exponentially (almost 45 minutes!), but will hopefully allow for a reverse geocoding call to succeed.

shankari commented 1 year ago

Line 9 ofrunAllTests.sh reads as follows:

PYTHONPATH=. python -m unittest discover -s emission/tests -p Test*;

This resulted in a python command not found error, because my machine uses python3. Changing the command to PYTHONPATH=. python3 ... fixed the error and allowed docker-compose to succeed. I may be able to fix future python incompatibilities by setting alias python='python3' in my .bash_profile.

This makes no sense. The whole point of running in docker is that it is isolated from what is on your machine. Even on your machine, if you use the version of python from the e-mission environment, you should not have to use python3, regular python should work

shankari commented 1 year ago

So we tried to run the container again using e-mission-server$ docker-compose -f setup/docker-compose-tests.yml up

It did appear to launch the tests, although we don't fully know why

We execed into the container with docker exec -it <container> /bin/bash and found that there was nothing in /usr/src/app

# cd /usr/src
# ls

where is this code running from then? Are we mounting the source code of the test somewhere?

nataliejschultz commented 1 year ago

Line 9 ofrunAllTests.sh reads as follows:

PYTHONPATH=. python -m unittest discover -s emission/tests -p Test*;

We tested out the docker-compose again and found that writing python3, while resolving the python command not found error initially, did not entirely resolve the problem.

We execed into the container with docker exec -it /bin/bash and found that >there was nothing in /usr/src/app

The server is in src/e-mission-server. Once in that directory, running source setup/activate.sh produced the error:

Could not find conda environment: emission

Running source setup/activate_conda.sh still did not allow the e-mission environment to run, so the python command was still not able to be run. Terminal suggests running conda info --envs to display the available environments, which produces these options:

base                     /root/miniconda-23.1.0
emissiontest             /root/miniconda-23.1.0/envs/emissiontest
shankari commented 1 year ago

@nataliejschultz Right, you need to activate emissiontest. again, try to run the start script commands one by one in the container and see what you get. Concretely, note

echo "Setting up the test environment..."
source setup/setup_tests.sh

echo "Running tests..."
source setup/activate_tests.sh

before running all tests

shankari commented 1 year ago

@nataliejschultz your issues with the local docker-compose are almost certainly a configuration issue on your side. As you can see, the CI test is running just fine, and I can verify that the tests are launched properly when I try to run.

$ docker-compose -f setup/docker-compose.tests.yml up
...
setup-web-server-1  | Found match for {'_id': ObjectId('64d2e0a02dec3b0cfd6065a7'), 'track_location': {'type': 'Point', 'coordinates': [-122.259169, 37.873873]}}
setup-web-server-1  | Found match for {'_id': ObjectId('64d2e0a02dec3b0cfd6065a8'), 'track_location': {'type': 'Point', 'coordinates': [-122.25874, 37.875711]}}
setup-web-server-1  | Found match for {'_id': ObjectId('64d2e0a02dec3b0cfd6065a9'), 'track_location': {'type': 'Point', 'coordinates': [-122.254577, 37.870352]}}
setup-web-server-1  | Setting up real example for 3c67379e-a214-402a-ab11-ea56ba822b97
setup-web-server-1  | Setting up real example for dc2bd19d-7ab5-4939-877e-a40ab384f6e9
setup-web-server-1  | Setting up real example for fbe2562c-d318-4741-985c-f8e93a5b2469
setup-web-server-1  | Setting up real example for 594f9d91-7245-4b16-a44c-a887887a8f9b
setup-web-server-1  | Setting up real example for 84fc72d5-ea49-49ff-8c7a-6fccee89ac21
setup-web-server-1  | Setting up real example for 775799e4-b6b7-4974-8ca2-f731d8d88a34
setup-web-server-1  | Setting up real example for d2eb9e84-44f9-43aa-8fbc-0d00f88cec3c
setup-web-server-1  | Setting up real example for 63efc7ab-1b20-4faa-bbf8-ac47364ba8d1
setup-web-server-1  | Setting up real example for 498f37ad-05c0-4d2f-80d7-1b0cc8dd9a6f
setup-web-server-1  | Setting up real example for 86edfbef-50ee-4659-8488-5b87a8eb03ce
setup-web-server-1  | Setting up real example for ca934fb5-1b3e-4bb9-a382-9d88762f6092
setup-web-server-1  | Setting up real example for 6fef7b65-6822-4faa-bf2c-4cc5f39ecb6b
setup-web-server-1  | Setting up real example for 9682c622-9ced-441d-b3f1-f97acabdec47
setup-web-server-1  | Setting up real example for e4d6af87-d5c9-4cda-becb-57f639230c3b
setup-web-server-1  | Setting up real example for 232bdddb-5f6e-484c-bf54-77036233d505
setup-web-server-1  | Setting up real example for bfe873f8-1b03-45a7-8725-8a179915182c
setup-web-server-1  | Setting up real example for 3a4b3bb8-7d2b-4d20-8f65-7bd82a0cf7c0
setup-web-server-1  | Setting up real example for 3c05b16d-12e4-4042-a5b7-bf0641da7b85
setup-web-server-1  | Setting up real example for dd808145-0e39-4c13-9b1b-1e45600cec7d

I would encourage you to clear your environment and re-test, following the instructions in the CI until you get this to work successfully.

nataliejschultz commented 1 year ago

I've found that docker desktop has a helpful interface for looking through the filesystem of a container:

Screenshot 2023-08-09 at 9 56 23 AM
nataliejschultz commented 1 year ago

I tried a few different fixes, including running in my emission environment, but nothing was working to resolve the error python: command not found when trying to run setup_tests.sh and setup_conda.sh. However, I noticed a message that kept showing up right before the error:

setup-web-server-1  | #
setup-web-server-1  | # To activate this environment, use
setup-web-server-1  | #
setup-web-server-1  | #     $ conda activate emissiontest
setup-web-server-1  | #
setup-web-server-1  | # To deactivate an active environment, use
setup-web-server-1  | #
setup-web-server-1  | #     $ conda deactivate

So, I added a single line: conda activate emissiontest to setup_tests.sh, right before it calls python. This resolved the issue locally.

I noticed that the same single line of code is present in activate_tests.sh, which is executed after both setup_tests.sh and setup_conda.sh. Is there a reason that the environments are activated in the order that they are? Could activating the environment earlier cause problems down the line, or potentially be beneficial to avoid future configuration issues?

nataliejschultz commented 1 year ago

Have spent the past hour trying to figure out why my reverse geocoding call wasn't working locally, but would work using nominatim directly. I was using curl -v http://localhost:8080/reverse?format=json&lat=37.8019701&lon=-122.419601,

and getting

{"error":{"code":400,"message":"Need coordinates or OSM object to lookup."}}. However, when calling in the same style exemplified in nominatim-docker's repo:

curl -v http://localhost:8080/search?q=street%20lombard the call succeeds. This verified that the issue was with the format of the reverse geocoding command. After changing things around with little success, I dug into the nomainatim-docker repo and found this page where someone was getting the same error.

Turns out that you need to put quotes around a call if it contains an ampersand while using curl. Finally succeeded with this:

curl -v "http://localhost:8080/reverse?format=json&lat=37.8019701&lon=-122.419601"

Now wondering if using postman would be a good option, if it's allowed?

nataliejschultz commented 1 year ago

Currently working on creating a fake trip in Rhode Island since I think the Northern California data will take too long to copy every time we test. Using the docker container method described in synthpop.

shankari commented 1 year ago

Now wondering if using postman would be a good option, if it's allowed?

I don't understand this comment. Where would we need to use postman? I can understand using curl for testing, but presumably we are using python code for reverse geocoding in production and that is what we are testing?

shankari commented 1 year ago

Using the docker container method described in synthpop.

Why do we need to use synthpop? We don't need to simulate a synthetic population. And there is no US Census data for Monaco anyway. Just pass in lat/lon points known to be in Monaco/Rhode Island/...

Note previous comment:

for an example of how to create a fake trip (you can use a similar method to create a fake place): emission/tests//analysisTests/userInputTests/TestConfirmedObjectFakeData.py or similar

shankari commented 1 year ago

This resolved the issue locally.

You should not have to make any code changes to resolve locally. If you do, you are not running the local system correctly. The CI/CD is running properly and so is my local copy.

noticed that the same single line of code is present in activate_tests.sh, which is executed after both setup_tests.sh and setup_conda.sh. Is there a reason that the environments are activated in the order that they are?

Because we need to setup before we activate. And once we have setup, we don't need to activate over and over. it is the same pattern that we use for setting up the non test environments.

You need to see where the python error is coming from. Again, I would suggest running the startup script in the docker container step by step so you can see where the error is coming from.

Think of unix/HPC. A program that runs locally but not on the remote server is not useful.

nataliejschultz commented 1 year ago

After our meeting yesterday, we found out that the python: command not found error that was arising did not impact the tests passing. Might be something to address later on, but it's not important at the moment for our purposes.

Currently looking at how to address the comments on my commits last night in the PR for this issue.

nataliejschultz commented 1 year ago

Haven't been logging progress very much as I was feeling very sick the past few days. Have been steadily working on addressing the comments, as well as issues I've encountered along the way. I spent time trying to understand how to get the environment variable for NOMINATIM_QUERY_URL to change in the testing environment, rather than changing the nominatim.py file. After finding out about "monkey patching", my view on variables was changed! I modified all of the query variables in the test files to eco.NOMINIATM_QUERY_URL, and this fixed my issue. Now, the query URL can be temporarily changed to the localhost:8080 URL required to run nominatim in the docker container during testing, but keep nominatim.json the same.

nataliejschultz commented 1 year ago

Testing locally from docker-compose.yml. I also modified the workflow file nominatim-docker-test.yml to run on push to see how far it would get into the workflow. After looking into the error:

ERROR: for nominatim  Cannot create container for service nominatim: No command specified

I ran docker image inspect mediagis/nominatim locally. Found that "Cmd": null. Not sure what to set the command to at this point, but looking into it.

nataliejschultz commented 1 year ago

Reading the dissertation and this part made me laugh :) Screenshot 2023-08-16 at 11 58 57 AM

nataliejschultz commented 1 year ago

Have been doing a lot of local testing. After reading up on docker compose documentation and doing some problem solving, I got the main server, mongo, and nominatim containers to run on different ports + install separately by creating a different network for nominatim. A big win!!

Have spent the past few hours trying to figure out why I'm getting the following errors:

CondaValueError: You have chosen a non-default solver backend (libmamba) but it was not recognized. Choose one of: classic

Above error shows up when running this line of code conda env update --name emissiontest --file setup/environment36.yml in setup_tests.sh. It does not occur when using docker compose to run the other tests, so I'm not sure what's going on. My initial thought is that it has to do with the locations of files that I've created.

Another error I'm getting is this: /start_integration_tests.sh: line 24: ./runIntegrationTests.sh: Permission denied

This is strange, because line 24 is not the run command that terminal is suggesting it is. This does not change despite my exiting the terminal and saving the file repeatedly. I've tried looking at the files within the docker containers and comparing them, but my server won't stay alive long enough to get into the emission code that I've copied. So, I haven't been able to look at the permissions for the file directly.

I'm wondering if the primary issue has something to do with the location of my docker-compose file. Other than that, I can't see major differences between my setup of the tests and the setup cascade that follows from test-with-docker.yml --> docker-compose.tests.yml.

shankari commented 1 year ago

A big win!!

🎉

CondaValueError: You have chosen a non-default solver backend (libmamba) but it was not recognized. Choose one of: classic

You made this change! https://github.com/e-mission/e-mission-server/pull/928

Again, if you are getting an error while running a docker container; docker exec -it into the container and debug

but my server won't stay alive long enough to get into the emission code that I've copied.

comment out the line that is running the tests and have the last command be tail -f /dev/null to keep the container up

nataliejschultz commented 1 year ago

This is strange, because line 24 is not the run command that terminal is suggesting it is.

Dug into the filesystem of the server container and it was using a file from the image created 8 days ago!! This page suggests it's a common issue and I can use docker-compose filename up -d --force-recreate --renew-anon-volumes during development in case files aren't updating. Deleting the image fixed it, but good to know in case docker uses old volume data in the future.

nataliejschultz commented 1 year ago

You made this change!

Refreshing the image appears to have also solved the libmamba issue.

Prior to our meeting, the workflow got to the point of trying the tests in GH actions, but not on my local machine. Thanks to Shankari, I was informed that this due to an HTTP error with conda. My machine could not download conda because of the VPN. Temporarily disabling the VPN and testing locally resolved this issue.

The next issue has to do with how nominatim is called in the container. Since the tests are initiated in the web server container, they cannot call nominatim using the.current url localhost:8080. I am going to try out changing the NOMINATIM_QUERY_URL to a url that calls the specific container, and then see what happens.

nataliejschultz commented 1 year ago

After replacing the query url, I've been trying to get the tests to pass locally. One of the test failures had to do with the mongo query, which I haven't tested yet. The other failures have to do with TestNominatim.py, and I'm really not sure what's going on. I've made multiple prints and tested locally about 20 times, but I can't figure it out. I'm going to commit, see how the tests run on GH actions, and try again tomorrow.

shankari commented 1 year ago

@nataliejschultz adding backtraces might help me give you pointers on how to resolve

nataliejschultz commented 1 year ago

I have gotten this error. pymongo.errors.ServerSelectionTimeoutError: localhost:27017: [Errno 111] Connection refused, Timeout: 30s, Topology Description: <TopologyDescription id: 64dfa64d35da3db595a7f950, topology_type: Unknown, servers: [<ServerDescription ('localhost', 27017) server_type: Unknown, rtt: None, error=AutoReconnect('localhost:27017: [Errno 111] Connection refused')

I've tried modifying the getURL function in TestMongodbAuth.py which currently runs as this in production:

def getURL(self, username, password, dynamo):
     return "mongodb://%s:%s@localhost/%s?authSource=admin&authMechanism=SCRAM-SHA-1" % (username, password, dbname)

I'm not as familiar with Mongo, so I'm not sure what I'm doing wrong. I've modified it in different ways to call the container that I'm running. An example of one I've tried:

def getURL(self, username, password):
     return "mongodb://%s:%s@db-1/1234?authSource=admin&authMechanism=SCRAM-SHA-1" % (username, password)

With the container being named db-1, running on port 1234.

shankari commented 1 year ago

TestMongodbAuth.py is not expected to pass. Please see the comments at the top of the file.

nataliejschultz commented 1 year ago

I see. It is included in the integration tests directory, which I thought you wanted to be run in its entirety. Do you want me to use a method other than unittest discover to run my tests specifically on GH actions? I thought the goal was to run all of the integration tests, but maybe I misinterpreted.

shankari commented 1 year ago

I wanted you to run your integration tests. We can eventually get TestMongodbAuth.py to work as well, but that is outside the scope of your work. You can just run your single integration test the way you do from the command line for testing.

nataliejschultz commented 1 year ago

As of my last commit, the tests are passing locally!! I think the error I'm getting in GH actions:

urllib.error.URLError: <urlopen error [Errno 111] Connection refused>

is due to the environment variable being set, since that's the main difference between my local and GH actions run. Going to troubleshoot a little bit and hoping this phase of it will be done!

shankari commented 1 year ago

@nataliejschultz by "tests are passing locally", do you mean that they are passing locally by running the same docker-compose command? Because in that case, there should be no difference between running it locally and running it in GH actions. That is what docker is supposed to ensure.

Again, details, such as "it works" versus "I get an error" can help me point you in the correct direction.

nataliejschultz commented 1 year ago

After extensive testing, trials, and errors, the tests are (almost) passing in GH actions. Initially, the tests would pass locally and give this error in GH actions:

urllib.error.URLError: <urlopen error [Errno 111] Connection refused>

I initially thought that I needed to fix the connection between the web and nominatim services, but that turned out to not be the issue. Instead, the problem was the inability of nominatim to initialize quickly enough.

I tried various changes to the docker-compose file:

Adding condition: service_healthy to the nominatim depends_on section;

Adding a healthcheck to the nominatim service,

    healthcheck:
      test: ["CMD", "curl", "-v", "http://localhost:8080"]
      interval: 1m
      timeout: 15s
      retries: 3

In tandem with restart: on-failure in the web-server service;

Changing the command in nominatim-docker-test.yml to read as docker-compose -d emission/integrationTests/docker-compose.yml up --wait, which is supposed to work with healthcheck to allow other services to wait until nominatim is healthy.

None of these potential solutions gave nominatim enough time to initialize from the ground up. Finally, I've added the following lines to start_integration_tests.sh:

echo "About to sleep! Zzzz..."
sleep 240
echo "Done sleeping! Running integration tests:"

And nominatim had just enough time to initialize. Currently, 4/6 of the tests are passing. This is a huge success in my eyes, because the current failures are assertion errors, and have nothing to do with the inability of the networks to connect/tests to run! Ideally, the delay of running the command would work through docker. For the time being, this shows me that running these tests with a docker container on GitHub actions in tandem with mongo functionality is possible. Once the fake trip/ground truth is functional, I don't think the assertion errors will be an issue.

nataliejschultz commented 1 year ago

@nataliejschultz by "tests are passing locally", do you mean that they are passing locally by running the same docker-compose command? Because in that case, there should be no difference between running it locally and running it in GH actions. That is what docker is supposed to ensure.

Yes, I mean I ran docker-compose -f emission/integrationTests/docker-compose.yml up and it resulted in:

Ran 6 tests in 0.186s

OK
shankari commented 1 year ago

was this with passing the env variable in through the docker-compose or in the Dockerfile? EDIT: I guess it is possible that nominatim is starting up more slowly on GH actions than it does on your local laptop.

I still don't see how you can get AssertionErrors on the GH actions but not locally

nataliejschultz commented 1 year ago

was this with passing the env variable in through the docker-compose or in the Dockerfile?

Both; passing the env variable in both places succeeds on my machine.

I still don't see how you can get AssertionErrors on the GH actions but not locally

I don't either. I kept having strange assertion errors locally, almost as if nominatim would return certain results in a different order sometimes. Still figuring out what was going on there.

nataliejschultz commented 1 year ago

Currently, two out of the 6 tests are failing. Specifically, they're failing with an assertion error, showing that the two place_ids are different. This is not occurring when I test locally, so it might be an issue specifically with non-updated place_ids in the docker container for Rhode Island. An example of the error message:

FAIL: test_get_json_geo (emission.individual_tests.TestNominatim.NominatimTest)
web-server_1  | ----------------------------------------------------------------------
web-server_1  | Traceback (most recent call last):
web-server_1  |   File "/src/e-mission-server/emission/individual_tests/TestNominatim.py", line 28, in test_get_json_geo
web-server_1  |     self.assertEqual(expected_result, actual_result)
web-server_1  | AssertionError: Lists differ: [{'place_id': 139763, 'licence': 'Data © OpenStreetMap co[399 chars]001}] != [{'place_id': 133953, 'licence': 'Data © OpenStreetMap co[399 chars]001}]
web-server_1  | 
web-server_1  | First differing element 0:
web-server_1  | {'place_id': 139763, 'licence': 'Data © OpenStreetMap co[398 chars]5001}
web-server_1  | {'place_id': 133953, 'licence': 'Data © OpenStreetMap co[398 chars]5001}
web-server_1  | 
web-server_1  |   [{'boundingbox': ['41.8237547', '41.8243153', '-71.4132816', '-71.4125278'],
web-server_1  |     'class': 'amenity',
web-server_1  |     'display_name': 'Providence City Hall, Fulton Street, Downtown, Providence, '
web-server_1  |                     'Providence County, 02903, United States',
web-server_1  |     'importance': 1.25001,
web-server_1  |     'lat': '41.824034499999996',
web-server_1  |     'licence': 'Data © OpenStreetMap contributors, ODbL 1.0. '
web-server_1  |                'https://osm.org/copyright',
web-server_1  |     'lon': '-71.41290469687814',
web-server_1  |     'osm_id': 121496393,
web-server_1  |     'osm_type': 'way',
web-server_1  | -   'place_id': 139763,
web-server_1  | ?                  ^^
web-server_1  | 
web-server_1  | +   'place_id': 133953,
web-server_1  | ?                 + ^
web-server_1  | 
web-server_1  |     'type': 'townhall'}]
shankari commented 1 year ago
  1. For now, I think that we should just skip comparing the place id, since everything else is the same.
    • once we have verified that that works, we can submit an issue against the the nominatim team asking them why it is not reproducible
  2. I do want to make sure that we are using the same tag for the docker container on your laptop and on GitHub actions, and we are loading the same versioned dataset. Any use of "latest" is going to affect our reproducibility.
  3. @nataliejschultz has the idea of comparing web call to our call. Let's think through what we want to test, and what the various options for doing this will let us test.
    1. use curl to talk to our docker container and use the python library to talk to our docker container and compare the results - is the library working correctly? Specifically that we have not made changes to the library that are a regression (that have broken something)
    2. use curl to talk to the web API and use our python library to talk to the web api - is the library working correctly and is it doing so against the current version of the web api? is it maintaining forwards compat
    3. use curl to talk to the web API and use our python library to talk to our docker container and compare the two - has the format/data of the web API response changed since we wrote out library?

which are useful?

shankari commented 1 year ago

poking around a bit more, the place ID is not part of the OSM data

Nodeid for Providence City Hall is 139763 and way 133953 does not exist and node 133953 was deleted 16 years ago
Screenshot 2023-08-22 at 2 56 08 PM Screenshot 2023-08-22 at 2 57 12 PM Screenshot 2023-08-22 at 2 59 43 PM
shankari commented 1 year ago

@nataliejschultz you don't even have to file an issue against nominatim https://duckduckgo.com/?t=ftsa&q=where+does+the+placeid+in+nominatim+come+from&ia=web says https://help.openstreetmap.org/questions/21542/nominatim-place-id

The place_id has no relevance for OSM, it is only an internal Nominatim identifier. Between different Nominatim instances, the place_id for the same OSM object may differ.

Please add this as a comment when you ignore the placeid in the check