Opened 2 years ago

Last modified 13 days ago

#25825 positive_review enhancement

Move cremona db to features

Reported by: gh-timokau Owned by:
Priority: major Milestone: sage-9.2
Component: build Keywords:
Cc: tscrim, fbissey, mkoeppe Merged in:
Authors: Timo Kaufmann, Jeroen Demeyer, Antonio Rojas Reviewers: Julian Rüth, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: u/arojas/cremona-db-features (Commits) Commit: 9c2dcb5badacc99f17e2c049ad51bf21589a8bc8
Dependencies: Stopgaps:

Description (last modified by mkoeppe)

This continues the work in #20382 and #25835.

To test this I ran ./sage -t --optional=sage,gap_packages,database_cremona_ellcurve $( git diff --name-only HEAD~2 ).

Next steps in getting rid of is_package_installed etc.:

  • #30746 sage.doctest.control: Replace use of sage.misc.package.list_packages
  • #30747 Deprecate all of sage.misc.package except for PackageNotFoundError

Change History (62)

comment:1 Changed 2 years ago by gh-timokau

  • Status changed from new to needs_review

comment:2 Changed 2 years ago by tscrim

  • Cc tscrim added

comment:3 Changed 2 years ago by gh-timokau

I just discovered #24718. This seems to be related/a duplicate. But that ticket seems to do some more unrelated stuff and probably needs a rebase. No clue why that is still "new".

comment:4 Changed 2 years ago by jdemeyer

In any case, I would rather have 1 ticket for each issue (1 for GAP, one for the Cremona database).

comment:5 Changed 2 years ago by gh-timokau

Sure, I'll split it up.

comment:6 Changed 2 years ago by gh-timokau

  • Branch changed from u/gh-timokau/use-features to u/gh-timokau/cremona-db-features
  • Commit changed from c13f429ac3074315020d5d0415baa72dca9362b2 to ca701d9a4ba3ed65338bc75095a2fa07ecb11b8d
  • Summary changed from Move gap_packages and cremona db to features to Move cremona db to features

New commits:

ca701d9Replace gap package checks with features

comment:7 Changed 2 years ago by git

  • Commit changed from ca701d9a4ba3ed65338bc75095a2fa07ecb11b8d to 1030350ee9cfd5729fca07b55f6e941530ee9fb0

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

1030350Replace remona db checks with features

comment:8 Changed 2 years ago by gh-timokau

  • Description modified (diff)

Split to this and #25835.

comment:9 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

As I mentioned on #24718: We should also use the database location discovered by the feature check instead of hardcoding it. In other words: there should be a single source for the database location.

comment:10 follow-up: Changed 2 years ago by gh-timokau

  • Dependencies set to #25309

What do you mean by "discover"? It looks to me like the path to the database is just as hardcoded in the feature. Am I reading that wrong?

That should probably use the environment variable I introduced in #25309 though. So that'll have to be merged first.

comment:11 in reply to: ↑ 10 Changed 2 years ago by jdemeyer

Replying to gh-timokau:

It looks to me like the path to the database is just as hardcoded in the feature.

Yes, it's hardcoded in two different places which is bad design. There should be single source for it.

comment:12 Changed 2 years ago by gh-timokau

I agree, I'll use the environment variable after #25309 is merged.

comment:13 Changed 2 years ago by gh-timokau

  • Milestone changed from sage-8.3 to sage-8.4

comment:14 Changed 2 years ago by git

  • Commit changed from 1030350ee9cfd5729fca07b55f6e941530ee9fb0 to e738ff5a01ef1b8104ada6ee3e9c6072a809d87a

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e738ff5Replace remona db checks with features

comment:15 Changed 2 years ago by gh-timokau

  • Status changed from needs_work to needs_review

Alright I'm using the proper environment variables now.

comment:16 Changed 2 years ago by jdemeyer

  • Dependencies #25309 deleted

My point was that you shouldn't use CREMONA_MINI_DATA_DIR, CREMONA_LARGE_DATA_DIR at all in the implementation, as the Feature already knows the filename.

comment:17 Changed 2 years ago by gh-timokau

Yes the filename, but not the directory. How is it supposed to know that?

comment:18 Changed 2 years ago by saraedum

I guess you should ask the feature for the path and not import it independently, see StaticFile.absolute_path().

comment:19 Changed 2 years ago by saraedum

  • Reviewers set to Julian Rüth
  • Status changed from needs_review to needs_work

comment:20 Changed 2 years ago by gh-timokau

  • Status changed from needs_work to needs_info

But how is the feature supposed to know the path?

comment:21 Changed 2 years ago by saraedum

The features looks at CREMONA_*_DATA_DIR. The point is that this information is only in the feature and users of the feature should not look at these variables again, i.e., the feature's implementation can change and we do not have to update all the users.

comment:22 Changed 2 years ago by saraedum

This is of course not so terribly important but it feels cleaner like that to me. (And closer to my idea of how features are supposed to work.)

comment:23 Changed 2 years ago by gh-timokau

  • Status changed from needs_info to needs_work

Ah, that makes sense to me. Also would make it easier to switch out the ad-hoc environment variables for a ./configure based system in the future. I thought Jeroen meant that I shouldn't use the environment variable at all.

comment:24 Changed 2 years ago by gh-timokau

In that case we'd still need CREMONA_MINI_DATA_DIR though, creating an assymetry. Should I change over anyways? Or make the two cremona databases mutually exclusive, always going through the feature and unifying the two env variables?

comment:25 Changed 2 years ago by gh-timokau

  • Status changed from needs_work to needs_info

comment:26 Changed 2 years ago by saraedum

Shouldn't both databases be features? Isn't the only difference that the mini one is a standard SPKG and the full one is optional?

comment:27 Changed 2 years ago by saraedum

  • Status changed from needs_info to needs_work

comment:28 Changed 2 years ago by gh-timokau

Yes, why would we use a feature to detect a standard package? Its always available by definition. Of course I would prefer to move more packages to optional and make sage more modular, but currently the mini db is standard.

comment:29 Changed 2 years ago by saraedum

It's a standard package in Sage-the-distribution but it still makes sense to use the feature infrastructure as it might make things easier for (us) packagers. Even if we always ship it we can rely on the features detecting the correct paths for things.

comment:30 Changed 2 years ago by gh-timokau

  • Cc fbissey added

I'm not sure if features are the right choice / intended for that use case. In general I think detecting time at runtime usually causes problems. Would that feature then only provide a path?

CC'ing fbissey because this is related to #25309, which apparently created problems for him.

comment:31 Changed 2 years ago by fbissey

Thanks for the concern but I am a proponent of using features more (been wanting to move to such thing for years), and the part of #25309 related to this ticket is fine by me.

comment:32 Changed 2 years ago by saraedum

I think it should be a feature in particular because we should not handle the large database differently from the mini database.

The feature would tell you whether the mini database is present (which must be true) and provide its path. I think for the cremona DBs that's all we need.

comment:33 Changed 2 years ago by gh-timokau

Alright and what should we do if the database is not present?

comment:34 Changed 2 years ago by saraedum

For the mini database: I would just ask for its absolute_path() which throws if it cannot find it. If that happens, I'd just let it throw. It won't happen on a properly configured setup. (and maybe add something like that as a comment in the code…)

Does that make sense?

comment:35 Changed 2 years ago by gh-timokau

Yeah that makes sense, I'll look into it.

comment:36 Changed 2 years ago by gh-timokau

Should I then remove CREMONA_{LARGE,MINI}_DATA_DIR entirely from env.py and instead read the variable directly (with fallback) in the feature?

That would have the advantage that there would be only one way to get that information. Otherwise, it will be easy for future code to use the variable from env.py instead.

But there would be the disadvantage that it wouldn't be as easy anymore to figure out which variables can be set to configure locations.

@fbissey as you were involved with env.py, do you have an opinion about that?

comment:37 Changed 2 years ago by git

  • Commit changed from e738ff5a01ef1b8104ada6ee3e9c6072a809d87a to d48c27da1843bcfd3dafc4aaa8552b299c73e26e

Branch pushed to git repo; I updated commit sha1. New commits:

d48c27dGet the cremona db path from the feature

comment:38 Changed 2 years ago by git

  • Commit changed from d48c27da1843bcfd3dafc4aaa8552b299c73e26e to 2dfba457e934a60536e663cde762a9184834d1c1

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2dfba45Get the cremona db path from the feature

comment:39 Changed 2 years ago by gh-timokau

  • Status changed from needs_work to needs_review

I added a mechanism to StaticFile that takes an environment variable as an override for search_path. It will then also notify the user of the existence of that search path when the file is not found. I like that solution even better than the env.py one.

For the record: I think resolution() should be refactored to return a list of lines for easier extensibility. The variable resolution should be removed because users can just override resolution() if they want to change the message. But that is out of scope of this ticket.

Also in the future I hope search_path_override could be replaced by a key in a configuration file instead of an env var.


New commits:

2dfba45Get the cremona db path from the feature

comment:40 Changed 21 months ago by fbissey

This appears to need some kind of rebasing.

comment:41 Changed 20 months ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:42 Changed 20 months ago by git

  • Commit changed from 2dfba457e934a60536e663cde762a9184834d1c1 to 68514344d6f1b043b22742c25b05ffd9078e9b6e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

65d9cd0Replace remona db checks with features
6851434Get the cremona db path from the feature

comment:43 Changed 20 months ago by gh-timokau

Rebased.


New commits:

65d9cd0Replace remona db checks with features
6851434Get the cremona db path from the feature

comment:44 Changed 19 months ago by fbissey

Are there any outstanding issues with this ticket or should we put it back to positive review?

comment:45 Changed 19 months ago by jdemeyer

For consistency, I would prefer if the handling of environment variables would remain in src/sage/env.py. Right now, you're moving part of that to src/sage/features/__init__.py

comment:46 Changed 19 months ago by gh-timokau

How would you suggest we do that? I'm not convinced hard-coding a search path override variable for every feature would be a better solution, but if you prefer that I'll implement it.

comment:47 Changed 19 months ago by jdemeyer

Maybe just use search_path=[CREMONA_MINI_DATA_DIR] in src/sage/features/databases.py? That wouldn't require any changes to the general feature framework.

comment:48 Changed 18 months ago by jdemeyer

Let's try to finish this... I'm having a look at it now.

comment:49 Changed 18 months ago by jdemeyer

  • Branch changed from u/gh-timokau/cremona-db-features to u/jdemeyer/cremona-db-features

comment:50 Changed 18 months ago by jdemeyer

  • Authors changed from Timo Kaufmann to Timo Kaufmann, Jeroen Demeyer
  • Commit changed from 68514344d6f1b043b22742c25b05ffd9078e9b6e to 2cf92e67460e705901f66905c5bed47550de47ce
  • Status changed from needs_work to needs_review

New commits:

966639bReplace remona db checks with features
2bc329aRemove search_path_override
808cc7bSearch for database in CREMONA_MINI_DATA_DIR and CREMONA_LARGE_DATA_DIR
20a3dedDeprecate set_global argument of CremonaDatabase
2cf92e6Clean up

comment:51 Changed 18 months ago by jdemeyer

The last 2 commits are not strictly needed, they are just cleaning stuff up. If a reviewer prefers to move that to a separate ticket, I'll do that.

comment:52 Changed 18 months ago by tscrim

I am just verifying the last two cleanup commits are good.

comment:53 Changed 15 months ago by arojas

  • Status changed from needs_review to needs_work

Rebase needed

comment:54 Changed 13 days ago by arojas

  • Branch changed from u/jdemeyer/cremona-db-features to u/arojas/cremona-db-features

comment:55 Changed 13 days ago by arojas

  • Authors changed from Timo Kaufmann, Jeroen Demeyer to Timo Kaufmann, Jeroen Demeyer, Antonio Rojas
  • Cc mkoeppe added
  • Commit changed from 2cf92e67460e705901f66905c5bed47550de47ce to 9c2dcb5badacc99f17e2c049ad51bf21589a8bc8
  • Status changed from needs_work to needs_review

Let's please try to get this finally in


New commits:

6b7abb1Merge branch 'develop' of git://git.sagemath.org/sage into t/25825/cremona-db-features
9c2dcb5More flexible test for error message

comment:56 follow-up: Changed 13 days ago by mkoeppe

Ticket description needs updating

comment:57 in reply to: ↑ 56 Changed 13 days ago by arojas

Replying to mkoeppe:

Ticket description needs updating

Is the OptionalExtension bit still relevant? I'm not familiar with the recent changes in that area.

comment:59 Changed 13 days ago by arojas

  • Description modified (diff)

comment:60 Changed 13 days ago by mkoeppe

  • Milestone changed from sage-8.4 to sage-9.2

comment:61 Changed 13 days ago by mkoeppe

  • Reviewers changed from Julian Rüth to Julian Rüth, Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:62 Changed 13 days ago by mkoeppe

  • Description modified (diff)
Note: See TracTickets for help on using tickets.