Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: evaluate profile_default #1767

Merged
merged 27 commits into from
Apr 19, 2024
Merged

fix: evaluate profile_default #1767

merged 27 commits into from
Apr 19, 2024

Conversation

jhaeu
Copy link
Contributor

@jhaeu jhaeu commented Mar 27, 2024

Pull Request Checklist

  • 1. I have rebased the latest version of the main branch into my feature branch and all conflicts
    have been resolved.
  • 2. I have added information about the change/addition to functionality to the CHANGELOG.md file under the
    [Unreleased] heading.
  • 3. I have documented my code using JDocs tags.
  • 4. I have removed unnecessary commented out code, imports and System.out.println statements.
  • 5. I have written JUnit tests for any new methods/classes and ensured that they pass.
  • 6. I have created API configuration tests for any new functionality exposed to the API.
  • 7. If changes/additions are made to the ors-config.json file, I have added these to the ors config documentation
    along with a short description of what it is for, and documented this in the Pull Request (below).
  • 8. I have built graphs with my code of the Heidelberg.osm.gz file and run the api-tests with all test passing
  • [ x 9. I have referenced the Issue Number in the Pull Request (if the changes were from an issue).
  • 10. For new features or changes involving building of graphs, I have tested on a larger dataset
    (at least Germany), and the graphs build without problems (i.e. no out-of-memory errors).
  • 11. For new features or changes involving the graphbuilding process (i.e. changing encoders, updating the
    importer etc.), I have generated longer distance routes for the affected profiles with different options
    (avoid features, max weight etc.) and compared these with the routes of the same parameters and start/end
    points generated from the current live ORS.
    If there are differences then the reasoning for these MUST be documented in the pull request.
  • 12. I have written in the Pull Request information about the changes made including their intended usage
    and why the change was needed.
  • 13. For changes touching the API documentation, I have tested that the API playground renders correctly.

Fixes #1762 .

Information about the changes

Reason for change: See issue #1762 . The root cause for the bug is, that properties that are set in application.yml ors.engine.profiles.* cannot be "overridden" by defaults defined in a user's ors-config.yml ors.engine.profile_default: The method merging profile properties and profile default properties does not know, if a profile property was defined in application.yml or in ors-config.yml, these files are already merged by spring. So properties from profiles.* always win.

This PR does not really fix the issue. Properties were moved from the application.yml's profiles section to profile_default. This was done for all properties where the same values were used for all profiles. The goal is, to not change behavior in existing ORS configurations if possible.

A real fix would be PR #1778 - but with the disadvantage, that users would have to change their enabled profiles in their ors-config.yml/env (see PR).

Or another real fix could later be implemented, e.g. defining defaults only in spring annotations in the code and generate ors-config.yml based on this.

Examples and reasons for differences between live ORS routes, and those generated from this pull request

Required changes to ors config (if applicable)

@takb takb added this to To do in ors general Mar 27, 2024
@github-actions github-actions bot added the fix label Mar 27, 2024
@jhaeu jhaeu force-pushed the fix/evaluate-profile-default branch from 5019edb to f6cdacc Compare April 18, 2024 13:08
to not block local ors runs
There seem to be issues when a graph built with yml config is read by ors with json config. So either -C have to be set to clear graphs before each test, which slows down, or to separate the tests. Further investigation needed.
@github-actions github-actions bot added fix and removed fix labels Apr 18, 2024
@github-actions github-actions bot added fix and removed fix labels Apr 18, 2024
@jhaeu jhaeu requested review from MichaelsJP and takb April 18, 2024 14:36
@github-actions github-actions bot added fix and removed fix labels Apr 18, 2024
@jhaeu jhaeu marked this pull request as ready for review April 18, 2024 15:13
@github-actions github-actions bot added fix and removed fix labels Apr 18, 2024
Copy link
Contributor

@takb takb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@takb takb force-pushed the fix/evaluate-profile-default branch from 8c9be1f to 6fcd8c3 Compare April 19, 2024 09:57
Copy link
Contributor Author

@jhaeu jhaeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@takb Why did you re-introduce enabled properties in all profiles? This change was intended by me, and it was discussed with @MichaelsJP .

@takb takb force-pushed the fix/evaluate-profile-default branch from a4bbcac to 13962c8 Compare April 19, 2024 11:48
@takb
Copy link
Contributor

takb commented Apr 19, 2024

@takb Why did you re-introduce enabled properties in all profiles? This change was intended by me, and it was discussed with @MichaelsJP .

just trying out things because the config conversion script is broken, no worries they're gone

Copy link

sonarcloud bot commented Apr 19, 2024

@jhaeu jhaeu merged commit ae528d3 into main Apr 19, 2024
38 checks passed
ors general automation moved this from To do to Awaiting release Apr 19, 2024
@jhaeu jhaeu deleted the fix/evaluate-profile-default branch April 19, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
ors general
  
Awaiting release
Development

Successfully merging this pull request may close these issues.

ors.engine.profile_default in ors-config.yml is ignored
2 participants