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

Feat/remove duplicated dependency declaration #54

Merged
merged 6 commits into from
Jun 7, 2024

Conversation

redur
Copy link
Contributor

@redur redur commented May 31, 2024

All dependencies are now directly managed in the pyproject.toml file.

Please consider the following:
Since we're developing a python package that is intended to use in combination with other python code and packages, we should keep the dependency restrictions as weak as possible. On the production side on the other hand, we want to fix all dependencies to fix the behavior of the production service.

We now made the shift from a fixed environment to declaring dependencies for the swissgeol-boreholes-dataextraction package. Therefore I tried to lift as many restrictions as possible. I believe we could further lift restrictions. I.e. do we really need python>3.10?
As we're the only users for now it is not an issue, but something to consider in the future. We also need to be careful, every time we notice some dependency issues, we have to adapt our restrictions in pyproject.toml.

Another note: I kept the restrictions for the dev dependencies rather fixed (I don't call them dev any more, but rather split them into test,lint,experiment_tracking & visualize. I haven't wrapped my head around if it really makes sense to have that many.
Test can be used for the testing CI.
Lint can be used for the lint CI.
Experiment tracking should be installed for local development (alongside with test and lint).
Visualize was something we used in a notebook, there's no need for a general user to install visualize. We could even think of removing this extra dependency alltogether.

Copy link

github-actions bot commented May 31, 2024

Coverage

Coverage Report
FileStmtsMissCoverMissing
src/stratigraphy
   __init__.py8188%11
   extract.py2102100%3–506
   get_files.py21210%3–48
   line_detection.py26260%3–76
   main.py91910%3–234
src/stratigraphy/util
   coordinate_extraction.py1172083%27, 47, 51, 55, 59–67, 88, 164, 184, 273, 276–277, 281, 293
   dataclasses.py32391%37–39
   depthcolumn.py2086768%26, 30, 51, 57, 60–61, 85, 88, 95, 102, 110–111, 121, 138–154, 199, 238, 254–262, 274, 279, 286, 313, 323, 352, 373, 376–387, 402–403, 448–490
   depthcolumnentry.py20480%12, 15, 27, 34
   description_block_splitter.py70297%24, 139
   draw.py73730%3–225
   duplicate_detection.py51510%3–146
   find_depth_columns.py89693%41–42, 70, 82, 175–176
   find_description.py632856%27–35, 50–63, 79–95, 172–175
   geometric_line_utilities.py86298%82, 132
   interval.py1075251%25–28, 32–35, 40, 45, 48, 100–146, 167, 172–188
   language_detection.py18180%3–45
   layer_identifier_column.py91910%3–227
   line.py492647%25, 42, 51, 65–95, 98
   linesquadtree.py46198%76
   plot_utils.py43430%3–120
   predictions.py1861860%3–386
   textblock.py74889%27, 51, 63, 75, 98, 119, 127, 155
   util.py402245%15–18, 22, 26, 40–47, 61–63, 87–88, 100–105
TOTAL1819105242% 

Tests Skipped Failures Errors Time
58 0 💤 0 ❌ 0 🔥 1.041s ⏱️

@redur
Copy link
Contributor Author

redur commented May 31, 2024

Note: Currently only local installation using pip install -e . For non local installation we need to find a way to provide the config files. I will open an issue for that.

@redur
Copy link
Contributor Author

redur commented May 31, 2024

#55

@redur redur force-pushed the feat/remove_duplicated_dependency_declaration branch from df3ace9 to edb6832 Compare May 31, 2024 14:25
@redur redur self-assigned this Jun 3, 2024
pyproject.toml Outdated Show resolved Hide resolved
]

[project.optional-dependencies]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the optional dependencies, we don't need to be that strict. I.e. we don't need to weaken the version as this is very much related to development and not for production purposes.

@redur
Copy link
Contributor Author

redur commented Jun 3, 2024

Since we look at the pyproject.toml file in this PR, I open the discussion about the package name.
Would it make sense to rename the package to boreholes-dataextraction? Or maybe swisstopo-boreholes-dataextraction? We could also think about shorter names.

Further, do we plan to publish the package on pypi at some point?

pyproject.toml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@stijnvermeeren-swisstopo
Copy link
Contributor

Since we look at the pyproject.toml file in this PR, I open the discussion about the package name. Would it make sense to rename the package to boreholes-dataextraction? Or maybe swisstopo-boreholes-dataextraction? We could also think about shorter names.

Further, do we plan to publish the package on pypi at some point?

  1. We could use swissgeol-boreholes-dataextraction. Also the script command (currently "boreholes-extract-layers") should probably be generalised, as we no longer only extract layers.
  2. No immediate plans yet, but it could be a logical step in the future. Would this choice have any influence on the development at this point in time?

@redur
Copy link
Contributor Author

redur commented Jun 6, 2024

Since we look at the pyproject.toml file in this PR, I open the discussion about the package name. Would it make sense to rename the package to boreholes-dataextraction? Or maybe swisstopo-boreholes-dataextraction? We could also think about shorter names.
Further, do we plan to publish the package on pypi at some point?

  1. We could use swissgeol-boreholes-dataextraction. Also the script command (currently "boreholes-extract-layers") should probably be generalised, as we no longer only extract layers.
  2. No immediate plans yet, but it could be a logical step in the future. Would this choice have any influence on the development at this point in time?
  1. For me it would make sense to divide the commands a bit further. Something like boreholes-extract-all, boreholes-extract-layersand boreholes-extract-metadata. This would go in line with the discussion we had about the architecture, and the possibility to redo certain steps given some constraints. I am fine with the name swissgeol-boreholes-dataextraction.
  2. We are preparing for that step already using pyproject.toml. However, at the moment the package is meant to be used using the source code as we provide the config file with the source code. I feel we need a solution for that problem anyways.

Rename package.
Rename command to extract information from borehole profiles.
Some fixes in the dependency declaration.
@redur
Copy link
Contributor Author

redur commented Jun 6, 2024

Comments are addressed. Let me know if it's all good now.

@redur
Copy link
Contributor Author

redur commented Jun 6, 2024

Follow up ticket in Jira is created.

@redur redur merged commit cc3ee87 into main Jun 7, 2024
3 checks passed
@redur redur deleted the feat/remove_duplicated_dependency_declaration branch June 7, 2024 13:50
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