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

Clean up plugin methods #347

Merged
merged 42 commits into from
Mar 8, 2023
Merged

Clean up plugin methods #347

merged 42 commits into from
Mar 8, 2023

Conversation

zcsahok
Copy link
Member

@zcsahok zcsahok commented Oct 3, 2022

As the base code got improved in the last year or so, it's time to somewhat clean-up and stabilize plugin code. If there are no objections then it could be even merged into master.

Current API has following methods:

  • init(param): called once upon start of TLF. It shall initialize or load any internal data. Currently param is a fixed empty string. The plan is to be able to pass information on the actual rule-set like category (in-state/ex-state), etc.
  • setup(): called when log is loaded or re-loaded. It shall reset any internal scoring-related data.
  • score(qso): evaluates QSO and returns an integer score. Available QSO fields currently are: band, call, mode, exchange.
  • check_exchange(qso): check QSO and can return a dictionary containing mult1_value (for multiplier counting) and/or normalized_exchange (to pretty-print the exchange). All return values are optional.

All methods are optional.

The API can be refined and extended in the future but as it stands it gives a stable base to start with. There also still open points in the code, but they are mostly related to error handling.

Contests like UBA #346 will require the special handling that can be added via a plugin.

@zcsahok zcsahok requested review from airween and dl1jbe October 7, 2022 19:31
@dl1jbe
Copy link
Member

dl1jbe commented Oct 7, 2022

Saw it already and review is on my schedule. Need some more time to think about. Hope to find time on weekend.

Drop 'nr_qsos' variable, use length of 'qsos_array' instead
@dl1jbe
Copy link
Member

dl1jbe commented Oct 14, 2022

As an start some unsorted items and questions I stumbled about while reading the code:

  • How TLF recognize that it should use .py for CONTEST=. What happens if is already hard coded? Seems that python module gets preference?
  • Do we use any non standard python modules yet or is an standard python installation sufficient (See comment from Martin)
  • When do we need to import 'tlf' module (see tesly.py) and when is it not needed (see raem.py)? Looks like it is imported anyway in 'plugin_init()'. So it could be dropped from any context.py?
  • plugin.c mentions PLUGIN_CONFIG parameter. What is the idea behind that CONFIG?
  • Is '#include "plugin.h"' in store_qso.c really needed?

If we leave aside the plugin specific code (will look it up later) it looks quite good so far. Handling and writing should be easy (if you are fluent in regular expressions).
Do we need any additional function to check if it may be a new mult? Or is that handled in TLF itself based on the 'check_exchange'?

There should be some documentation (maybe a README howto use the functionality) before merging into the master branch. Are some of the 'FIXME' statements critical and have to be fixed before merging?

Anyway good work. Looks promising.

@dl1jbe
Copy link
Member

dl1jbe commented Oct 15, 2022

Another point:

  • The plugin provides some kind of API to the rule script - the 'qso' and the 'tlf' structure, 'get_qrb_for_locator'.
  • That set of data and functions will surely grow with time and with requests by contest rules.

So we need some kind of versioning for it. Otherwise someone with an old TLF version may be trying to use rule scripts written for newer plugin version.

In the script there should be something like 'Required_Version' or similar.

@zcsahok
Copy link
Member Author

zcsahok commented Oct 15, 2022

Thanks for the review!

How TLF recognize that it should use .py for CONTEST=. What happens if is already hard coded? Seems that python module gets preference?

It tries to load the file rules/<contest>.py. If it is missing then there is no plugin present. A plugin provides just two optional functions for scoring and mult determination. Both take precedence over the internal logic (updated getexchange). Defining a plugin for a built-in contest (like CQWW) generally doesn't make too much sense (if that was the question). The general idea is to use it only for the cases when our internal logic can't describe the rules.

Do we use any non standard python modules yet or is an standard python installation sufficient?

It's standard python, for Debian python3-dev is needed. If a plugin need some specific module, it's up to them. (normally just regexp would suffice, I guess)
What python version does your OS have? There is an issue that could be nicely solved in 3.9, so it would be better to have 3.9 as minimal version.

import tlf

Yes, it can be dropped from py file.

PLUGIN_CONFIG parameter

See the initial comment: The plan is to be able to pass information on the actual rule-set like category (in-state/ex-state), etc.

"plugin.h"' in store_qso.c

Leftover, dropped.

Do we need any additional function to check if it may be a new mult?

This is done in TLF internally, if you mean the housekeeping of mults. It is however assumed that plugin provides a legit mult value, it is used as-is.

FIXMEs

I was focusing on the good case, some error handling branches are left open. They are not critical.

we need some kind of versioning

One option would be to let init() return an (optional) integer for the required version number and TLF would exit if it's too high. If nothing is returned then no check is made.

@zcsahok
Copy link
Member Author

zcsahok commented Oct 15, 2022

Documentation

Will be added to the manual as a last step of this PR.

@dl1jbe
Copy link
Member

dl1jbe commented Oct 19, 2022

Thanks for the explanations.

How TLF recognize that it should use .py for CONTEST=. What happens if is already hard coded? Seems that python module gets preference?

It tries to load the file rules/<contest>.py. If it is missing then there is no plugin present. A plugin provides just two optional functions for scoring and mult determination. Both take precedence over the internal logic (updated getexchange). Defining a plugin for a built-in contest (like CQWW) generally doesn't make too much sense (if that was the question). The general idea is to use it only for the cases when our internal logic can't describe the rules.

Ok, So try to find a plugin and if not found fall back to conventional behavior.

Do we use any non standard python modules yet or is an standard python installation sufficient?

It's standard python, for Debian python3-dev is needed. If a plugin need some specific module, it's up to them. (normally just regexp would suffice, I guess)

And as far as I can see regex module is part of the python standard distribution.

What python version does your OS have? There is an issue that could be nicely solved in 3.9, so it would be better to have 3.9 as minimal version.

At the moment we are at 3.10 here. 3.11 will be our standard version early next year.

PLUGIN_CONFIG parameter

See the initial comment: The plan is to be able to pass information on the actual rule-set like category (in-state/ex-state), etc.

Ok. So a simple string to pass and interpretation lies in the responsibility of the plugin.

Do we need any additional function to check if it may be a new mult?

This is done in TLF internally, if you mean the housekeeping of mults. It is however assumed that plugin provides a legit mult value, it is used as-is.

Ok. I see. So nothing should be needed. Fine.

we need some kind of versioning

One option would be to let init() return an (optional) integer for the required version number and TLF would exit if it's too high. If nothing is returned then no check is made.

Or the other way round. Make the init function in rules.py mandatory and provide it with the support version nr in the 'tlf' module. The let it check itself and return true or false depending upon that number being sufficient.

Only question: Do we want to use the normal TLF version number or do we need an additional API version? I think normal version number should fit.

We should also think which information about the qso may be used by newer 'creative' contest rules. I remember such things as being allowed to work a station in different time slots of the whole contest more than once. So it may be appropriated to add some more information to the qso structure , e.g. the time and date. But again, that may be add in a later version using the mechanism already in place.

@zcsahok
Copy link
Member Author

zcsahok commented Oct 25, 2022

Re API version check: we could use TLF version number. It has the advantage that it's more transparent to the end user ("This plugin requires TLF 2.1 but you are running 1.9~git ..."). The check is better done in TLF core as comparing two version numbers is somewhat involved (we don't want each plugin author to come up with their own check method) and this way we also can give a consistent error message.
On the other hand this requires that we take care of our version number. Best would be to bump to 1.6 with this PR.
I'm preparing the version check.

@dl1jbe
Copy link
Member

dl1jbe commented Oct 25, 2022

Re API version check: we could use TLF version number. It has the advantage that it's more transparent to the end user ("This plugin requires TLF 2.1 but you are running 1.9~git ..."). The check is better done in TLF core as comparing two version numbers is somewhat involved (we don't want each plugin author to come up with their own check method) and this way we also can give a consistent error message.

Consistent error message can be done in TLF also if we use true/false return. but I agree it would be good to have a common check function. So the init function should return the minimal version number (as string?).

On the other hand this requires that we take care of our version number. Best would be to bump to 1.6 with this PR. I'm preparing the version check.

Why 1.6? The next number would be 1.5. If we integrate the plugin code before next release it would be fine. We should only guarantee that 1.5~git is scored as below 1.5. Or do I miss a point here?

@zcsahok
Copy link
Member Author

zcsahok commented Oct 27, 2022

I mixed up the 1.6, so all is fine. 1.5~git is in fact pre-1.5 and thus treated the same as 1.5.

Yes, init shall return the minimal required version number as a string.
E.g. with type hints:

def init(cfg: str) -> str:
    return '1.9'

It's also fine to return None if there is no specific requirement. Or even leave init out.

Changed showstring()'s arguments to const as the compiler was complaining.

zcsahok and others added 6 commits November 3, 2022 09:15
Add California QSO party rules

- Sending leading zeros is discouraged in CQP
- Add custom cabrillo qso format for cqp
* in addition migrate NO_LEADING_ZEROS_SERIAL to LEADING_ZEROS_SERIAL
@zcsahok
Copy link
Member Author

zcsahok commented Jan 5, 2023

The basic idea is to return the contents of the dxcc_data struct resulting from dxcc_by_index(getctydata(call)). There is a strong overlap between dxcc_data and prefix_data, so I have to see if the infos from two can be merged.

@dl1jbe
Copy link
Member

dl1jbe commented Jan 5, 2023

From http://www.country-files.com/cty-dat-format/ and the code in dxcc.c you can see that dxcc_data represents the major country information which eventually gets overridden (with more precise information) from the following prefix entries (in prefix_data).

E.g. long and lat in dxcc_data are for center of country and the same in prefix_data are specific for the prefix area. Similar for timezone, itu / cq zone and others.

The prefix entries all link back to the dxcc country info via the 'dxcc_ctynr' struct member.

So maybe the prefix data are most relevant for the plugin. They should be completed by missing information from the dxcc_data entry. As far as I can see the dxcc entry give the following additional info:

  • Countryname as string
  • The primary dxcc prefix as string
  • The 'starred' info which tells if the entry counts in WAEDC or ARRL list (See format info column 70 in the link above´)

@zcsahok
Copy link
Member Author

zcsahok commented Jan 6, 2023

Added Dxcc structure combining dxcc_data (as main_* fields) and prefix_data. The method get_dxcc() returns a Dxcc object corresponding to its argument.

Example: the result of tlf.get_dxcc('PT2A') has these fields (among others)
country_name='Brazil'
main_prefix='PY'
main_itu_zone=15
prefix='PT2'
itu_zone=13
starred=False

Also exposed my.call as tlf.MY_CALL.

A HA-DX test log:
image
Will check in the rules later.

@zcsahok
Copy link
Member Author

zcsahok commented Jan 7, 2023

Moved Hamlib log level setting before initialization to avoid output like

Using initial exchange file hadx.txtlocator2longlat called

@dl1jbe
Copy link
Member

dl1jbe commented Jan 11, 2023

Added Dxcc structure combining dxcc_data (as main_* fields) and prefix_data. The method get_dxcc() returns a Dxcc object corresponding to its argument.

Looks good and should serve all our purposes.
+1

@VictorDenisov
Copy link
Contributor

How do I compile it?
I'm getting this error:

plugin.c:49:8: error: unknown type name ‘PyStructSequence_Desc’
   49 | static PyStructSequence_Desc qso_descr = {
      |        ^~~~~~~~~~~~~~~~~~~~~

I enabled python plugin with
./configure --enable-python-plugin

@zcsahok
Copy link
Member Author

zcsahok commented Jan 14, 2023

The configure call is correct. Here is the output I get

...
checking whether to build Python plugin support... yes
checking for a Python interpreter with version >= 3.9... python3
checking for python3... /usr/bin/python3
checking for python3 version... 3.9
checking for python3 platform... linux
checking for python3 script directory... ${prefix}/lib/python3.9/site-packages
checking for python3 extension module directory... ${exec_prefix}/lib/python3.9/site-packages
checking for python3.9-config... /usr/bin/python3.9-config
checking python include flags... -I/usr/include/python3.9 -I/usr/include/python3.9
checking python libs... -lpython3.9 -lcrypt -lpthread -ldl  -lutil -lm -lm 
...

Did you try it on a fresh clone?

@VictorDenisov
Copy link
Contributor

VictorDenisov commented Jan 14, 2023

It turned out I needed to install python-config that matches my python version 3.10. For some reason my debian doesn't have python3.10-config, but it allowed me to change python-config to use python 3.10 version instead of 2.7.

Also I would suggest to document that python plugin file name is derived from the name of the CONTEST in the logcfg.dat I initially thought that the name is the same as the rules files. I took the example from hadx. In hadx it probaby works because hadx doesn't have CONTEST field.
It's not clear what relationship is there between CONTEST field and rules file names.

@zcsahok
Copy link
Member Author

zcsahok commented Jan 14, 2023

I also ran into that issue with CONTEST. CONTEST and RULES are currently synonyms, most likely due to historical reasons, but the two have different uses. Will open an issue to sort this out.

@VictorDenisov
Copy link
Contributor

Just tested this feature in NA qso party. Works like a charm. Can't wait to have it in the master branch! When it's merged I'll have naqp rules for you.

@zcsahok
Copy link
Member Author

zcsahok commented Jan 27, 2023

Updated man page for get_dxcc().

Fixed HA-DX: score for own country has to be specified.

Copy link
Member

@airween airween left a comment

Choose a reason for hiding this comment

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

Not for this PR (because the mentioned line was added in a previous comment), but for general review: the band member of the qso object is always 0, seems Tlf never fill it.

Copy link
Member

@airween airween left a comment

Choose a reason for hiding this comment

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

I have two notes for this PR, let me know if I can help anything.

src/plugin.c Show resolved Hide resolved
@zcsahok zcsahok mentioned this pull request Mar 6, 2023
@zcsahok
Copy link
Member Author

zcsahok commented Mar 6, 2023

If there are no conceptual problems or other major issues then my preference would be to merge this into master.

@airween airween self-requested a review March 6, 2023 07:17
Copy link
Member

@airween airween left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@dl1jbe
Copy link
Member

dl1jbe commented Mar 6, 2023

Just go on with the merge Zoli. I think all critical problems got sorted out back in january.

@zcsahok zcsahok merged commit 6c9c89e into Tlf:dev_plugin Mar 8, 2023
zcsahok added a commit that referenced this pull request Mar 8, 2023
Add Python plugin support (#347)

Some functions can be overridden by contest specific plugin methods.

---------

Co-authored-by: Victor Denisov <[email protected]>
Co-authored-by: Thomas Beierlein <[email protected]>
Co-authored-by: Thomas Beierlein <[email protected]>
Co-authored-by: Jonathan Bastien-Filiatrault <[email protected]>
@zcsahok
Copy link
Member Author

zcsahok commented Mar 8, 2023

Merged dev_plugin into master.

@zcsahok zcsahok mentioned this pull request Mar 20, 2023
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.

5 participants