lincc-frameworks / python-project-template

Python project best practices for scientific software
https://lincc-ppt.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
61 stars 15 forks source link

Copier doesn't preserve dependencies when updating to a newer template version #119

Closed drewoldag closed 1 year ago

drewoldag commented 1 year ago

This was first reported by @dougbrn in issue #116 .

We should investigate this further - it sounds like it would make for a very bad user experience if a projects dependencies are wiped out when updating to a new version of the template.

drewoldag commented 1 year ago

I'm having some trouble reproducing this error. These are the steps that I've followed.

drew@pterodactyl new_science % git diff pyproject.toml 
diff --git a/pyproject.toml b/pyproject.toml
index c37bd3c..0acd395 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -32,9 +32,7 @@ dev = [
     "sphinx==6.1.3", # Used to automatically generate documentation
     "sphinx_rtd_theme==1.2.0", # Used to render documentation
     "sphinx-autoapi==2.0.1", # Used to automatically generate api documentation
-
     "pylint", # Used for static linting of files
-
 ]

 [build-system]
@@ -46,13 +44,3 @@ build-backend = "setuptools.build_meta"

 [tool.setuptools_scm]
 write_to = "src/example_module/_version.py"
-
-
-
-[tool.pylint.'MESSAGES CONTROL']
-disable = """
-    missing-module-docstring,
-"""
-ignore-patterns = "^_.*" # Ignore files that start with an underscore, i.e. _version.py
-
-
(scratch) drew@pterodactyl new_science % 
drewoldag commented 1 year ago

@dougbrn I'm having some trouble reproducing the effect that you noticed in issue #116. Do you happen to have any other details of the process that you could share?

I know it's hard to remember key strokes from a week ago, but do you happen to have those written down by any chance?

I also took a look at the following PRs, but they don't really show the problem (I don't think):

drewoldag commented 1 year ago

From here: https://github.com/lincc-frameworks/lsstseries/pull/48/commits

There are these two commits:

Are those showing what you saw?

The last one shows dependencies being added back in, but the one immediately prior doesn't show them being removed, as near as I can tell. Though my git archeological skills are not great :)

dougbrn commented 1 year ago

Hmm, yes I really wish I had taken better notes on my process. I don't have a clear set of command that could reproduce the issue, and what you have here probably demonstrates that this isn't a real issue when everything is done correctly. Two possible thoughts:

  1. When you ran copier --vcs-ref=HEAD update; # This takes the latest template on main, did you have to walk through the copier questions again? I believe I remember having to fill out the form again, and provided some different answers than in the previous template most likely. This potentially could have clobbered the existing toml file? But if you ran with no issues even when walking through the copier questions then that may not be it
  2. In your example you've gone from v1.2.1 to HEAD, I think lsstseries went from v1.1.0 to ~HEAD. Not sure if that would make any difference in terms of the amount of work that's gone on in the last month.

But overall, I really wouldn't be surprised if this was some kind of user error. If neither of my above thoughts are useful in reproducing things it might be enough to close this for now and we can reopen if it ever comes up again? Hopefully with better notes. Thanks for taking a thorough look into it.

drewoldag commented 1 year ago

Thanks for your comments!

To your first point there, I'm pretty sure that I just took all the default answers, and didn't make any changes in the process. I did the custom installation, so I saw all the questions.

I'll take a look at the difference between 1.1.0 and HEAD. There's likely some substantial changes in the toml file between those two reference points.

drewoldag commented 1 year ago

I'm still not able to reproduce this. So I'm going to close this for now. We'll definitely reopen and examine in more detail if we hear more reports about this.