Skip to content

Revert documentation changes - issue needs to be fixed in e-mission-server#1139

Closed
Copilot wants to merge 4 commits intomasterfrom
copilot/fix-1128
Closed

Revert documentation changes - issue needs to be fixed in e-mission-server#1139
Copilot wants to merge 4 commits intomasterfrom
copilot/fix-1128

Conversation

Copy link

Copilot AI commented Aug 21, 2025

This PR reverts the documentation changes that were incorrectly added to explain workarounds for read-only wrapper properties. After investigation, the root cause is more nuanced than initially understood.

Root Cause Analysis

The issue last_place.location = start_loc.loc fails while last_place.exit_ts = start_loc.ts works because:

  • Both location and exit_ts are WORM (Write-Once Read-Many) properties
  • exit_ts hasn't been set yet on the place object, so it's writable
  • location was already set earlier (likely in stitch_together_end), making it read-only

The problem occurs because last_place represents a place created at the end of a previous trip, so its location is already populated, but its exit_ts is being set for the first time when the next trip begins.

Solution

The actual fix needs to be implemented in the e-mission-server repository at:
https://github.com/e-mission/e-mission-server/blob/84334681c3cb630d873e56046869249d1f67650b/emission/analysis/intake/segmentation/trip_segmentation.py#L382

Change:

last_place.location = start_loc.loc

To:

last_place["location"] = start_loc.loc

This bypasses the read-only wrapper by using dictionary-style assignment instead of direct property assignment.

This is not a documentation issue but a code logic issue that requires changes to the server-side trip segmentation algorithm.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copy link
Author

Copilot AI commented Aug 21, 2025

@shankari 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs.

I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review.

Copilot AI and others added 2 commits August 21, 2025 00:39
…ting

Co-authored-by: shankari <2423263+shankari@users.noreply.github.com>
Co-authored-by: shankari <2423263+shankari@users.noreply.github.com>
Copilot AI changed the title [WIP] Trip Segmentation: AttributeError: property location is read-only Add documentation for working with read-only wrappers in data model Aug 21, 2025
Copilot AI requested a review from shankari August 21, 2025 00:42
@shankari
Copy link
Contributor

@copilot the goal here is to make changes to the e-mission/e-mission-server repository, not improve the documentation here

Copy link
Author

Copilot AI commented Aug 21, 2025

@copilot the goal here is to make changes to the e-mission/e-mission-server repository, not improve the documentation here

I understand the goal is to make changes to the e-mission-server repository. However, I'm currently working in the e-mission-docs repository.

Could you clarify:

  1. Should this PR be moved to the e-mission-server repository?
  2. Or should I make code changes here that would then need to be applied to e-mission-server?
  3. What specific changes in e-mission-server are needed to fix the read-only wrapper issues?

I can revert the documentation changes I made here if they're not the right approach.

@shankari
Copy link
Contributor

@copilot also, I added this comment to the underlying issue, but you didn't see it (at least there was no emoji)

Copy link
Author

Copilot AI commented Aug 21, 2025

@copilot also, I added this comment to the underlying issue, but you didn't see it (at least there was no emoji)

You're absolutely right - I understand now that the actual fix needs to be in the e-mission-server repository, not in the documentation. I added documentation here explaining the workaround, but that doesn't solve the production issue.

I don't have access to make changes in the e-mission-server repository from here. Could you clarify:

  1. Should I revert the documentation changes I made here?
  2. How should I proceed to make the actual code fix in e-mission-server (changing last_place.location = start_loc.loc to last_place["location"] = start_loc.loc on line 382)?

I can see the issue is specifically at https://github.com/e-mission/e-mission-server/blob/84334681c3cb630d873e56046869249d1f67650b/emission/analysis/intake/segmentation/trip_segmentation.py#L382 but I need guidance on how to access that repository to make the fix.

@shankari
Copy link
Contributor

@copilot

Could you clarify:

  1. Yes
  2. You cannot, since this repo does not have any code
  3. You need to look at the server code around https://github.com/e-mission/e-mission-server/blob/84334681c3cb630d873e56046869249d1f67650b/emission/analysis/intake/segmentation/trip_segmentation.py#L382
    last_place.exit_ts = start_loc.ts
    last_place.exit_local_dt = start_loc.local_dt
    last_place.exit_fmt_time = start_loc.fmt_time
    last_place.starting_trip = curr_trip_entry.get_id()
    if "enter_ts" in last_place:
        last_place.duration = last_place.exit_ts - last_place.enter_ts
    else:
        logging.debug("Place %s is the start of tracking - duration not known" % last_place)
        # Since this is the first place, it didn't have its location set at the end of a trip
        # in stitch_together_end. So we set it here. Note that this is likely to be off by
        # a bit because this is actually the start of the trip, but it is not too bad.
        last_place.location = start_loc.loc

the failure is at last_place.location = start_loc.loc but we use the same pattern earlier as well (e.g. last_place.exit_ts = start_loc.ts) why are they not failing? what is the underlying cause?

@shankari
Copy link
Contributor

Should I revert the documentation changes I made here?

Yes

How should I proceed to make the actual code fix in e-mission-server (changing last_place.location = start_loc.loc to last_place["location"] = start_loc.loc on line 382)?
but I need guidance on how to access that repository to make the fix.

The other repo is also open source. what additional guidance do you need?

Co-authored-by: shankari <2423263+shankari@users.noreply.github.com>
Copy link
Author

Copilot AI commented Aug 21, 2025

@copilot

Could you clarify:
...

I've reverted the documentation changes as requested (commit 607bccd → 7da2b95).

Looking at the e-mission-server code, I found the underlying cause:

The location property fails while exit_ts works because both are WORM (Write-Once Read-Many) properties, but:

  • exit_ts hasn't been set yet, so it's writable
  • location was already set earlier (likely in stitch_together_end), making it read-only

The issue occurs because last_place represents a place that was created at the end of a previous trip, so its location is already set, but its exit_ts is being set for the first time when the next trip begins.

The actual fix would need to be in the e-mission-server repository's trip_segmentation.py around line 382.

Copilot AI changed the title Add documentation for working with read-only wrappers in data model Revert documentation changes - issue needs to be fixed in e-mission-server Aug 21, 2025
@shankari
Copy link
Contributor

shankari commented Aug 21, 2025

@copilot can you please generate a PR in that repository?

1 similar comment
@shankari
Copy link
Contributor

@copilot can you please generate a PR in that repository?

Copy link
Author

Copilot AI commented Aug 21, 2025

Should I revert the documentation changes I made here?

Yes
...

I've already reverted the documentation changes in commit 1cf24cb.

Regarding the e-mission-server repository - I can analyze the issue and provide the exact code changes needed, but I cannot directly create a PR in another repository since I only have access to this e-mission-docs repository. I can provide the specific fix that needs to be applied to line 382 in trip_segmentation.py.

@copilot can you please generate a PR in that repository?

I cannot directly create a PR in the e-mission-server repository since I only have access to this e-mission-docs repository. However, I can provide the exact fix needed:

In /emission/analysis/intake/segmentation/trip_segmentation.py around line 382, change:

last_place.location = start_loc.loc

to:

last_place["location"] = start_loc.loc

This bypasses the read-only wrapper issue since location was already set when the place was created at the end of the previous trip.

@shankari shankari closed this Sep 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments